Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: add info on migration from tymon's package #56

Merged
merged 3 commits into from
Nov 23, 2021
Merged

docs: add info on migration from tymon's package #56

merged 3 commits into from
Nov 23, 2021

Conversation

leon0399
Copy link
Member

Close #54

@mfn
Copy link
Contributor

mfn commented Nov 20, 2021

Thanks, was looking for this!

Will give the steps a try and report back!

@mfn
Copy link
Contributor

mfn commented Nov 20, 2021

Quick first impression of the guide:

  • after running composer update I got this in the end; still need to check if this is specific to my setup or not:
    > Illuminate\Foundation\ComposerScripts::postAutoloadDump
    > @php artisan package:discover
    
    In LaravelServiceProvider.php line 7:
    
      Class 'Tymon\JWTAuth\Providers\LaravelServiceProvider' not found
    
    
    Script @php artisan package:discover handling the post-autoload-dump event returned with error code 1
    
  • I've a custom implementation of JWTGuard and need a) extend and b) override it via a custom serviceprovider:
    class LaravelServiceProvider extends BaseLaravelServiceProvider
    {
        /**
         * Override parent so we can use our own JWTGuard class
         * Extend Laravel's Auth.
         */
        protected function extendAuthGuard(): void
        {
            $this->app['auth']->extend('jwt', function ($app, $name, array $config) {
                $guard = new JWTGuard(
                    $app['tymon.jwt'],
                    $app['auth']->createUserProvider($config['provider']),
                    $app['request']
                );
    
                $app->refresh('request', $guard, 'setRequest');
    
                return $guard;
            });
        }
    }
    This fails because the constructor arguments are not compatible anymore (the Dispatcher was recently added)

@mfn
Copy link
Contributor

mfn commented Nov 20, 2021

One downside of running composer update this way is that it will upgrade all packages. This may be undesired if one just wants to replace this one.

README.md Outdated
Comment on lines 8 to 9
1) Replace `"tymon/jwt-auth": "^1.0"` with `"php-open-source-saver/jwt-auth": "^1.2"` in your `composer.json`
2) Run `composer update`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One downside of running composer update this way is that it will upgrade all packages

I did some tests to figure out what would have the least amount of impact on existing packages:

  1. composer update works, but updates all packages which may not be desired
  2. Running first composer remove tymon/jwt-auth and then composer require php-open-source-saver/jwt-auth seems to work nicely, one does not even have to manually touch the composer.json
  3. Keeping manually changing the composer.json but still avoiding updating unrelated packages, this also worked in my test: composer update php-open-source-saver/jwt-auth --with-dependencies
    Since tymon/jwt-auth is not referenced in composer.json, this will remove it too

My preference would be 2): even though it's not as nice looking and is essentially two commands, it does not require manually changing anything and will automatically chose the best version to install (^1.2 currently) so is "maintenance free" insofar we don't need to update the reference in the README regarding the version number:

Suggested change
1) Replace `"tymon/jwt-auth": "^1.0"` with `"php-open-source-saver/jwt-auth": "^1.2"` in your `composer.json`
2) Run `composer update`
1) Run `composer remove tymon/jwt-auth`
2) Run `composer require php-open-source-saver/jwt-auth`

WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll re-check once again, might be possile, that it is needed to add --no-scripts, since if composer will trigger package discovery, it can throw an error about non-existing classes

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my case, special snowflake scenario, I've actually disabled the auto-discovery for the (previous name of the) package because I register my own serviceprovider, which extends the one from the package.

Copy link
Contributor

@eschricker eschricker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 👍

Copy link
Contributor

@mfn mfn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It LGTM but since this adds a "Migration" chapter, we should mention the incompatibility of the constructor I pointed out in #56 (comment) and we should probably strive to make the 1.* series not less compatible

@leon0399 leon0399 requested a review from eschricker November 23, 2021 10:51
@Messhias
Copy link
Collaborator

Look good for me too.

@eschricker eschricker merged commit 709e9b0 into PHP-Open-Source-Saver:develop Nov 23, 2021
@mfn
Copy link
Contributor

mfn commented Nov 23, 2021

❤️

Thank you 🙏🏼

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make documentation on how to migrate from original repository
4 participants