User API

I think it's a good suggestion to add some basic session information to all ce-api responses. We just need to make sure it does not bloat responses. Also, we should do it like with local-tasks, after the main response rendering, so it does not hurt cache-ability of main responses.

It could be added via a separate, optional module, but that seems like unnecessary (module) bloat to me, so simply adding it in the main ce-api module seems fine to me.

I'd propose the following addition.

  $data['current_user'] = [
    'id' => $user->id(),
    'name' => $user->getDisplayName(),
    'roles' => array_keays($user->getRoles(),
  ];

Thus, some slight changes:
- uid -> id -> less drupal-isms for JS-folks
- remove "is-admin", that check can be done easily in JS imo

Imo this would be a great, straight-forward addition. I think we can just add it + add some short addition to the tests to ensure it's added correctly.

Of course I agree with you 100%!!

@fargo just adding a tested patch here to make sure I have done this correctly?

Thanks! However the patch has a problem with a cachability, without adjusting cache-metadata this can become cached wrongly, e.g. consider dynamic-page-cache using the same cache for multiple users.

To fix, I think we should add this late after dynamic page cache, as we do with other dynamic content (local tasks) already: https://git.drupalcode.org/project/lupus_ce_renderer/-/blob/2.x/src/Even...

Thus, when we add here a similar responsesubscriber and same wait for adding session information, we should be good. Could you change it like that?

also, please add your patch as merge requests, so we can have the automated test suite run + add some short addition to the tests to ensure it's added correctly.

Yup no worries! I will review the cache and do a pull request as suggested in the correct place.

great!
To clarify what I think to be the right place: lupus_decoupled_ce_api module + EventSubscriber/CurrentUserResponseSubscriber.php that works like https://git.drupalcode.org/project/lupus_ce_renderer/-/blob/2.x/src/Even...

@fago I have gone ahead and created a merge request:
https://www.drupal.org/project/lupus_ce_renderer/issues/3506828 ✨ Add basic user session info to ce-api Active

@glynster:

Thank you, but you put it into the wrong module. As said before I think the right place is here:

> To clarify what I think to be the right place: lupus_decoupled_ce_api module

Thus, it needs its own event-subscriber class like EventSubscriber/CurrentUserResponseSubscriber.php

Reason: LupusCeRenderer is a generally usable module and NOT an opinionated setup. The opinionated setup is here in lupus_decoupled, so this is the right place to add optional stuff that we think make sense, but are no existing core feature - like this one.

Could you move it over? Then we need to extend some of the test-cases to verify the output is right also.

@fago sorry for the confusion. Added a new subscriber, tested and works! MR is ready.

I think that provided MR works fine, I'd just like to highlight the other way of adding data to ce-api responses - adding the lupus_ce_renderer_response_data request attribute. It could also go into custom code:

  public function addBasicUserInfo() {
    $request = $this->getCurrentRequest();
    $lupus_ce_renderer_response_data = $request->attributes->get('lupus_ce_renderer_response_data');
    $lupus_ce_renderer_response_data['current_user'] = $this->getCurrentUserInfo();
    $request->attributes->set('lupus_ce_renderer_response_data', $lupus_ce_renderer_response_data);
  }

Additionally, there is hook_lupus_ce_renderer_response_alter as well.

@useernamee this is what we are currently doing lupus_ce_renderer_response_data for various things thanks for the tip!

Let me know if there is anything else I can do to help here.

Thanks!

The MR looks good, agreed!

Some small things:

- https://git.drupalcode.org/issue/lupus_decoupled-3503738/-/jobs/4374454 shows phpcs issues
- I think we should comment on the event-subscriber weight, why is set to 10. It should explain we take care of running after dynamic page cache.
- Then, we need to add test coverage. Could you take care of that? I think we should be able to extend some of the existing tests to also check that the user-session data is returned correctly.

@fago I have fixed the phpcs issues and added some tests. Let me know.

Thank you, that looks great!

The test case is not a functional test though, so I moved it into a new Kernel folder. It might even work as unit test, but I did not bother trying / asking for this to become a unit test. That seems to be a minor optimization which should not hold things up. If it works as unit test, it could be improved in a follow-up issue if you or someone likes. But as said, I think it's ok as is!

Testing this now on gitlab1

Works like a charm! Merged, thank you! :-)

Automatically closed - issue fixed for 2 weeks with no activity.