Skip to content

[2.x] Change $page.user to $page.auth.user #317#323

Closed
drjdr wants to merge 1 commit into
laravel:1.xfrom
drjdr:feature-inertia-auth-user
Closed

[2.x] Change $page.user to $page.auth.user #317#323
drjdr wants to merge 1 commit into
laravel:1.xfrom
drjdr:feature-inertia-auth-user

Conversation

@drjdr
Copy link
Copy Markdown
Contributor

@drjdr drjdr commented Oct 7, 2020

  • Changing $page.user to $page.auth.user makes authenticated user consistent with intertia.js examples.
  • Prevents mixing the auth.user with a user object returned from backend.

@taylorotwell
Copy link
Copy Markdown
Member

Major breaking change for little benefit?

@drjdr
Copy link
Copy Markdown
Contributor Author

drjdr commented Oct 7, 2020

It will require a quick code change for the early adopters indeed.
However

  • A quick change/win now...
  • it will prevent unexpected behaviour for every person that returns a user to an Inertia::render
  • A clear distinction between the authenticated user and the user in the view, while maintaining consistency in variable names.

I understand that this change in the crucial variable name is going to break stuff, but hence the [2.x] label

update:

@peckanthony
Copy link
Copy Markdown

I just had this same issue, while trying out Jetstream with Inertia and creating a user list and detail page as a HelloWorld app.

I understand this might be a breaking change now, but I'm willing to bet a lot people will encounter this. I didn't even notice it at first, only when I saw the name of the current logged in user being changed in the navigation bar.

I think an authUser or auth.user prop would prevent this issue and be more consistent with the Laravel backend auth user.

@danilopinotti
Copy link
Copy Markdown

What about made these changes as a config? Or just allow publishing InertiaSharedData to make it easier to make these changes

@drjdr
Copy link
Copy Markdown
Contributor Author

drjdr commented Dec 21, 2020

@taylorotwell @driesvints @reinink May I kindly ask to reconsider to add this in v2? Having to substitute user with user_something on every page to avoid interference with the auth.user seems not right...

@drjdr drjdr mentioned this pull request Dec 21, 2020
@reinink
Copy link
Copy Markdown
Contributor

reinink commented Dec 22, 2020

For your info, @taylorotwell and I had a chat about this yesterday, and I think we're going to make this change after-all. Taylor was (rightfully) trying to avoid a breaking change here, but the reality is we've already got a breaking change, since the $page object in the Vue adapter has been updated to include all the properties of the page object (component, props, url, version). Meaning, the current user will now be access via $page.props.user. And, since this is a breaking change either way, we might as well just do $page.props.auth.user right away as well. 👍

Both @claudiodekker and myself are working hard to update both Inertia and the Vue adapters in preparation for the Jetstream 2.0 release. We'll also be making some PRs against Jetstream (likely today at some point) to get everything in sync and ready to go.

@driesvints
Copy link
Copy Markdown
Member

Thanks @reinink!

@drjdr
Copy link
Copy Markdown
Contributor Author

drjdr commented Dec 22, 2020

Thanks a lot to all of you and the work you do for this project!

@drjdr
Copy link
Copy Markdown
Contributor Author

drjdr commented Dec 23, 2020

Hi @reinink @claudiodekker ,
While thinking about the consequences of the above, we have 3 components that will be used on every front-end page: user, team (current_team) and permissions. It would be good to structure them nicely before the release of v2.

auth : {
    user : {},
    permissions : [{}],
    team: {},
}
  1. auth.user instead of user to get the current user, and fixes interference with returned user. (this PR)
  2. auth.permissions: On the vuePage Pages/Teams/UpdateTeamNameForm.vue we can see an example of permissions being passed to this component. But permissions are needed everywhere. Adding permissions also to auth makes 1 uniform location for all permissions in your applications. It would also prevent a similar issue arising when you want to make a permissions manager where you need to pass the permissions as a prop. On the Controller, one can just merge permissions to this auth.permissions array for now, but in a v3 of jetstream we could even extend this with a few helper classes in Laravel and vue (similar to https://laravel.com/docs/8.x/authorization#via-blade-templates). Doing this now will prevent braking changes in the future and make this more uniform for all developers.
  3. auth.team: This would be a simplification. Currently there is a $page.props.user.current_team, but it could be an option to have it in $page.props.auth.team ( as there is always only one current team, and we neither name auth.user -> auth.current_user).

@markvesterskov
Copy link
Copy Markdown

@reinink Did you choose not to implement this change for V2 after all? I can see in the ShareInertiaData middleware that the User prop has not been moved to a new auth property.

@reinink
Copy link
Copy Markdown
Contributor

reinink commented Jan 7, 2021

@markvesterskov UGH, we totally forgot! 🤦

@taylorotwell Probably too late for Jetstream V2, yes? Or do you think it would be possible to still make this change? 😬

@peckanthony
Copy link
Copy Markdown

Please do! I think this issue will be reposted every few weeks otherwise, I've seen a few others like this already

@markvesterskov
Copy link
Copy Markdown

@reinink I noticed because of me overwriting the user model in a controller today making everything blow up. I reeeheeeeeally hope that this can still make it in! If not I’ll have to hack it together myself and that would be a shame.

@reinink
Copy link
Copy Markdown
Contributor

reinink commented Jan 7, 2021

It's really @taylorotwell's call at this point, if he feels like this makes sense to bring to V2 still.

I suspect it's actually fine, since once you install Jetstream, it's "done".

Meaning, if you've installed V2 already, you'll get the current behaviour. But if we update it to do auth.user instead, those who install it after that will get that behaviour.

I don't think anything in the docs actually refers to user (correct me if I'm wrong), so there is no "breaking change" here really.

@taylorotwell
Copy link
Copy Markdown
Member

taylorotwell commented Jan 7, 2021

@reinink if we make the change the next time an existing application updates their Composer dependencies their templates will break. I'm OK with making the update (I've heard enough complaining about Jetstream so what's a little more?) but we will need to make a post explaining that people need to make the change in their templates.

@bretto36
Copy link
Copy Markdown

Is this still under review? Or did something actually occur

@timavo
Copy link
Copy Markdown

timavo commented Jan 23, 2022

Any update on this? I really would like to see this getting updated. :-)

@SamuelMwangiW
Copy link
Copy Markdown
Contributor

Hi @driesvints,

I propose to reopen this issue and maybe affected users can attempt to PR.
Thanks for the amazing work!

@driesvints
Copy link
Copy Markdown
Member

We can't do this change in Jetstream 2.x for reasons explained above. Please attempt this PR to master.

@SamuelMwangiW
Copy link
Copy Markdown
Contributor

SamuelMwangiW commented Jun 22, 2022

Well noted Dries.

I will attempt the PR to master. Thanks.

In the meantime and for anyone who may stumble this issue in future, here is how I worked around.

I used the container to bind modified copy of the middleware as follows:

<?php

namespace App\Providers;

use App\Http\Middleware\ShareInertiaData;
use Illuminate\Support\ServiceProvider;
use Laravel\Jetstream\Http\Middleware\ShareInertiaData as JetstreamShareInertiaData;

class InertiaDataServiceProvider extends ServiceProvider
{
    public $bindings = [
        JetstreamShareInertiaData::class => ShareInertiaData::class,
    ];
}
<?php

namespace App\Http\Middleware;

use Illuminate\Support\Facades\Gate;
use Illuminate\Support\Facades\Session;
use Inertia\Inertia;
use Laravel\Fortify\Features;
use Laravel\Jetstream\Jetstream;

class ShareInertiaData
{
    /**
     * Handle the incoming request.
     *
     * @param \Illuminate\Http\Request $request
     * @param callable $next
     * @return \Illuminate\Http\Response
     */
    public function handle($request, $next)
    {
        Inertia::share(
            array_filter([
                'jetstream' => function () use ($request) {
                    return [
                        'canCreateTeams' => $request->user() &&
                            Jetstream::hasTeamFeatures() &&
                            Gate::forUser($request->user())->check('create', Jetstream::newTeamModel()),
                        'canManageTwoFactorAuthentication' => Features::canManageTwoFactorAuthentication(),
                        'canUpdatePassword' => Features::enabled(Features::updatePasswords()),
                        'canUpdateProfileInformation' => Features::canUpdateProfileInformation(),
                        'hasEmailVerification' => Features::enabled(Features::emailVerification()),
                        'flash' => $request->session()->get('flash', []),
                        'hasAccountDeletionFeatures' => Jetstream::hasAccountDeletionFeatures(),
                        'hasApiFeatures' => Jetstream::hasApiFeatures(),
                        'hasTeamFeatures' => Jetstream::hasTeamFeatures(),
                        'hasTermsAndPrivacyPolicyFeature' => Jetstream::hasTermsAndPrivacyPolicyFeature(),
                        'managesProfilePhotos' => Jetstream::managesProfilePhotos(),
                    ];
                },
                'auth' => [
                    'user' => function () use ($request) {
                        if (!$request->user()) {
                            return;
                        }

                        if (Jetstream::hasTeamFeatures() && $request->user()) {
                            $request->user()->currentTeam;
                        }

                        return array_merge(
                            $request->user()->toArray(),
                            array_filter([
                                'all_teams' => Jetstream::hasTeamFeatures() ? $request->user()->allTeams()->values(
                                ) : null,
                            ]),
                            [
                                'two_factor_enabled' => !is_null($request->user()->two_factor_secret),
                            ]
                        );
                    },
                ],
                'errorBags' => function () {
                    return collect(optional(Session::get('errors'))->getBags() ?: [])->mapWithKeys(
                        function ($bag, $key) {
                            return [$key => $bag->messages()];
                        }
                    )->all();
                },
            ])
        );

        return $next($request);
    }
}

PS: Note that by overriding a the above class, you are responsible to track changes upstream. You also need to update $page.props.user to $page.props.auth.user or whatever structure you choose

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.

10 participants