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.
Given there will be more layers, like https://github.com/StirStudios/stir_nuxt_base/tree/release/nuxt-ui-lupus, I think we should add support for adding nuxt-layers via some optional environment variable to gitpod-links. Then we can easily build links which auto-add the layer as needed.
For example an optional variable FRONTEND_NUXT_LAYER=github:StirStudios/stir_nuxt_base#release/nuxt-ui-lupus could add the respective nuxt-layer to a nuxt-based frontend. I'm adding a sub-task to discuss and clarify details there.
Thank you! but as pointed out at the parent task, I think this should go into lupus_decoupled!
@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.
thank you! I reviewed the PR and left a couple of remarks there, please check.
Also, please always provide Gitpod-test links for your branches when creating a PR. You need it anyway to test it yourself, so please be so kind and provide the link to reviewers as well.
thx, tested both PRs via gitpod. Much better! Mattia reviewed and approved already.
Merged, thank you!
https://github.com/drunomics/lupus-decoupled-nuxt3-demo/commit/37810a54a...
https://github.com/drunomics/lupus-decoupled-nuxt3-demo-shadcn/commit/28...
I created the list of unified hosting options: https://github.com/drunomics/lupus-decoupled-website/commit/4b86ecf35bfb...
What's left is only
* #3490828: Add a Drupal introduction page
* Add page of "starters" below nuxt
which each have a dedicated issue. Given that, I think we can the current state of the docs "solid", call this done and add those two pages in the mentioned issues as follow-up.
Works all fine! Login & Logout, node editing, all good.
I merged changes from the naked repo into shadcn repo: https://github.com/drunomics/lupus-decoupled-nuxt3-demo-shadcn/commit/04...
Testing now.
A new core release with the fix is out, I think we can call this fixed!
Successfully tested the MR, it resolves the issue while responsive preview integration still works.
ContentSecurityEventSubscriber sets both frame-ancestor and frame-src CSP headers, but only frame-src is necessary to make responsive-toolbar work. the other one seems out of scope and creates this issue. Since there is no reason to set it in this module, imo it's best to remove it.
it seems the header is set by
modules/lupus_decoupled_responsive_preview/src/EventSubscriber/ContentSecurityEventSubscriber.php
Could we improve the naked demo with some little css added by default to forms, maybe just some margins?
For the shadcn demo, could we configure tailwindforms globally somehow such that the form looks decent.
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...
thanks! recipes should be able to require v3.
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.
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?
Verified, the error is gone!
The fix works! Let's also auto-enable layout builder and mere this MR when the CE release is out.
tested it with lupus decoupled - this solves the problem! Code looks great also, thus merged!
https://www.drupal.org/project/drupal/releases/11.1.2 →
is out, so the patch is not necessary any more.
We could set a dependency, but that makes it require 11.1, while atm you could also install older core versions like 10.4. So let's better simply leave it out.
https://github.com/drunomics/lupus-decoupled-project/commit/aba0d9701f90...
MR is ready. I ran into one related, but separate problem with layout_builder though, so creating bug report.
> ATM I am working on a webform comp and field render. I prefer to keep all the API as json and allow the frontend to render as needed.
makes sense, but how does that tie into the admin layer? Imo the admin improvements should be its own thing, the webform thing can be as well!
> Get feedback and testing and that would be one way I can help?
that would be definitely helpful, yes!
While I can see the use-case for needing this information, I don't think it makes sense to ship this information on every ce-api page response, since it would grow all the responses, while the information is usually only needed once. Thus, it seems to be better solved with a dedicated endpoint + a separate request to fetch it from nuxt. When done so, we shuold be able to leverage caching in nuxt such that the request is done only once in the server instance, and cached then.
On how to providing the information, I don't think there is a pre-existing API for it. So it makes sense to add it.
However, when adding it: The question becomes, what exactly shall we add? It seems very use-case specific what shall be added, there are definitely more config-settings that one might want to use in the frontend, for example a configured google analytics key.
Thus, I feel like we could build a solution for exposing any kind of config as "Site-info" and make it configurable, e.g. by specifying the keys you want:
system.site:name
system.site:slogan
system.site:mail
and just output a data structure like
{ system.site : {
name: "..",
slogan: "..",
mail: "..."
}}
and make the result a public API, e.g. at /lupus/site-info
I guess, that would make sense to provide as a new optional module, Lupus Decoupled Site Info ?
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.
Discussed this with arthur_lorenz and agreed that atm the most effective, short-term solution is to start with a dedicated layer which we could ship for drupal-cms, thus following solution:
> B) Ship with suiting CE-output config in our recipes + ship suiting frontend components for them. We could ship frontend components by providing a simple nuxt layer that does nothing more than providing those components + add this nuxt-layer in our naked and shadcn demos.
I don't think it's the job of lupus decoupled to add menus like footer-menu, because it's very solution specific. However, it makes a lot of sense in some recipe or demo-recipe.
We started building recipes like https://www.drupal.org/project/lupus_decoupled_recipe → - atm the focus is on support recipes of drupal cms, but we could also add some small additions I think. Does drupal cms have a footer menu?
I think in the end we'll have combinations of recipes shipping config + nuxt layers / repos shipping suiting components frontend. Together we have a way to ship re-usable solutions / starters. We have some discussion on how to do it best at 📌 Ship additional vue components for Drupal CMS Active - I'd suggest we work together there to work it out and then start leveraging the same solution!
yes, there was a bug in how core handled CSRF tokens in the menu when provided via API. I tracked it down and solved it a while ago, see 🐛 Menu APIs provide invalid CSRF tokens Active . Meanwhile, fortunately the patch was merged into core, so it will be part of next core patch releases! (all of 11.1.x, 10.5.x, and 10.4.x., 11.x)
Another idea: What about some drush command that generates a vue component for a given content type and view mode?
I already had the idea in mind to create some UI to make it easy for people to copy and paste the starter-component, so a drush command to make the same seems to be a good first step.
Still, the drush command does not solve the "copy things to the frontend" part. We could write some bash script and ask people to run this to take care of it, and/or maybe add a drupal recipe action to take care of that part.
Finally, just generating vue components probably ain't enough, to make things nice we probably want to add some styles anyway. That makes me thing that for now the most straight forward option would to provide drupal-cms vue components in a simple nuxt layer which just ships those components. The nuxt layer we could just add to our demo setup, since it's a simple config that people can easily remove when not needed.
there is also related 📌 Provide Lupus Decoupled as recipe Active
lupus_decoupled_form is a generic module providing support for forms generically - it does not make sense for it to contain module specific things. That said, I wonder - https://git.drupalcode.org/project/antibot/-/blob/2.0.x/src/AntibotFormA... - is thtat data- attribute not handed over to the frontend? If so, that might be a *general* issue we should fix.
But looking at the anitbot module file, it seems there is a bit of more stuff we need to make sure we need to support, so we might want to add a lupus_decoupled_antibot module to make sure it works.
But the greater question is how to ship the frontend part. One way would be solving 📌 Enabling loading Drupal JavaScript via support for drupal-libraries Active what I'd love to do asap, but might take us some time to get right.
A more straight-forward approach right now is to work-out a nice approach for having custom vue-components for drupal-cms deployed to the frontend. That will be needed for other things anyway. Then, we need this work with that, e.g. we could have some special drupal-antibot custom element which we ship that way + our integration module takes care of adding for those forms. Then the frontend component can take care of loading the Drupal antibot JS library and handling the necessary glue code.
yes, alterantive-A with customized default config is great!
- Let's stop using legacy processors for things like this! They are mostly around for BC, we should adapt the new API for new things!
My first thought would be to add multi-value support to the "Flattened" formatter, so that its output becomes non-flattened for multi-value fields
yeah, I see no point in configuring a "flattened" formatter when the data is multiple. We cannot flatten it then.
That said, the configured default behaviour for multiple image/media fields should mimic the one for single ones, but instead of a single, flattend JS object it should give us an array of similar objects. Can we configure that? Then we should simply have default-config generated based upon the field cardinality and use flattening only for single-value fields.
. But I know you've spoken out against that, so I'll instead copy some code into FileCeFieldFormatter to support the two cases, and possibly make it not extend FlattenedCeFieldFormatter anymore.
hm, the "flattening" issue seems generic, thus we should better avoid building it into FileCeFieldFormatter - this would mean we would have to build it into many plugins in the end imo. Can we solve it by generating suiting config as suggested above instead?
However,
1 this fact is not visible in the UI. You only see the one "default" (auto-created) CE display, and no tab/local tasks for "Full" etc.
2 as soon as you save the default CE display, this behaviour changes. Custom elements in 'full' view mode will be bult based on the default CE display. This also IMHO is the only logical behaviour.
imo this is totally fine.
However, 2. is not perfect for many use cases, either. In a standard Drupal install / lupus_decoupled gitpod, it would mean that as soon as you save the "default" CE display for an article, the "RSS" and "Teaser" CE displays are immediately saved with it (so that custom elements output does not change for those view modes). You very likely do not want "RSS" to be saved, because you just don't care about that view mode.
yes, that'S a bad idea. We shall not auto-save more view-modes.
The current behaviour + message seems totally fine, and I don't see a need to mess with it. It's reliable and also makes sense: In the beginning you only see "default" and it's obvious that is what will be applied to all of them. Does the "default" also have the checkboxes for which view-modes to customize in the bottom? So you need to save that anyway when you want to customize any of those, so seems good?
after commenting this, I realize that changing this won't help our default output since by default the body field *has* a summary enabled!
Maybe we should go back and instead of doing this simply improve the default-config for our body field? Could we use the "flatten" plugin to flatten the output in two slots, like summary and body/default slot? If not, I'd be fine with simply only output the full-body field by default in the slot and not handling the summary. Folks can still change the config if the summary shall be used somehow.
+1 on changing it like this, also on changing in the way as proposed in the MR. Still I added some remarks!
if ($element->hasSlotNormalizationStyle($slot_key, CustomElement::NORMALIZE_AS_SINGLE_VALUE)
|| (ThisSlotOnlyHasASingleValue($slot_key) && MyFormatCannotHandleArrayValues()) ) {
$slot_data = reset($slot_data);
}
This pseudo-code does not make sense to me. It'S not that Vue cannot handle that array, it's that it does not make much sense to render into a single Vue component via a slot. The body is typically rendered using a simple slot, what semantically makes sense, but that's done with something like this pseudo-code: <component content="$SLOT"></component> $SLOT must be string then and will fail it's an array. Thus, the ARRAY fails because our frontend-code excpects a string.
That said, it seems we had this working before - although it's not so logical - and it makes frontend code more consistent to have it working again. Because of that, I turned around and fixed the frontend, so it IS working now.
Anyway, I think we shall do this chance still, because we want to have a default output which is semantically good. And this is without the array.
--> I re-title this a bit.
Still, before merging - I think we need test coverage here. Ideally we could extend the unit test to cover it, but howsoever, we shall make sure we have this behavior covered by tests. thus setting needs work for that.
the module is already installed in gitpod, so that line can be skipped.
Let's do a separate command then for enabling optional modules in the demo in gitpod + add it to README docs for regular ddev users. Let's enable responsive-preview + rest-debug module also, I think it's also there.
Works now! Thus, merged the update to the naked-demo :-)
ok, finally got it to run. test results:
node/preview/* route failed, the route is wrongly at /preview/* at the moment. after fixing this it works.
node/1/layout-preview seems to work fine
created fix at https://github.com/drunomics/nuxtjs-drupal-ce/pull/304 and testing this now.
ok, this is completed and ready to be tested. Here is the change with the update, which shall fix this.
https://github.com/drunomics/lupus-decoupled-nuxt3-demo/pull/68
- The MR cannot be merged, because it conflicts. Please update and let's merge then
- I added a remark to the docs update, please check. https://github.com/drunomics/lupus-decoupled-website/pull/94#pullrequest...
Updated todos:
* #3490828: Add a Drupal introduction page
* Add page of "starters" below nuxt
* Add list of unified hosting companies
This is done :-) -> https://lupus-decoupled.org/deployment/deployment-strategy
I disagree. Yes, the existing LupusSessionDomainFunctionalTest test tests the user login, but not via our form integration. It's using the core JSON API for logins, see https://git.drupalcode.org/project/lupus_decoupled/-/blob/b637b316157b5e...
Imo the critical parts we need to cover here is submitting and reloading forms, as pointed out at https://www.drupal.org/project/lupus_decoupled/issues/3481050#mr101-note... 📌 Add tests for lupus_decoupled_form Active The requests the frontend would send to the backend can be seen here: https://github.com/drunomics/nuxtjs-drupal-ce/blob/2.x/src/runtime/serve... - it's a regular POST request with form-data towards `/ce-api/user/login`. The result has to be a valid custom elements response, e.g. a json page object for rendering the result-page or a json redirect resonse (not a http redirect).
Test of option A was quite positiv, tested both latest firefox and chrome, with default privcacy options (firefox) enabled + an active adblocker. No problems a all, when nuxt ssr is set to false, CSR kicks in and the preview works just fine.
Thus, option A is viable! We need to make sure the frontend applies CSR and disables the server-proxy for the preview routes so the API communication happens direct. We can set some custom route-rule for CSR, for the disabling of the server-proxy we miss he per route option. So this might need some work in nuxtjs-drupal-ce to make it work.
The LupusCSR theme solution adds quite some additional complexity, since it is requiring both the theme and a nuxt server to be there and working.
Let's explore other options to find somehing simpler:
Option A) Use client-side rendering for preview routes and directly access the backend API, so he cookie is sent. Needs the working CORS setup and might face some problems with privacy-aware browsers not sending the cookie to a 3rd party cookie domain
Option B) add a drupal /proxy/, which proxies the frontend SSR and displays the frontend under the backend domain. This needs some JS/CSS rewriting for the new base URL so they are loaded from an absolute path..
Optiona A would be probably the simplest, since it is re-using things which are already exising, the CORS submodule and CSR feature of nuxt. Let's give it some test with latest browsers. With chrome not killing 3rd party cookies this might be good enough.
re-titling and raising prio
I cannot reproduce the described behavior. If it really happens, it certainly qualifies as bug, thus fixing the category. Please provide steps to reprodocue, e.g. with gitpod test environments.
Quick review:
* I don't think the module README is the place for drupal-CMS documentation. The README should stay general and document how to use it with Search API. But concrete drupal-CMS instructions should go elsewhere imo (not sure where), or better be not necessary since automated, but that'S a different story/issue.
* Cannot judge about the details, but let's make sure things work via automated tests. Can we also add some search-api & views config to tests that we run as part of automated tests? I think we could simply copy things over from the recipe, but I'd copy to document what is supported on the long-time and not depend on the recipe since the recipe can change over time.
so this is about supporting search api views - let's make this clearer.
I figured multi-frontend and redirect API responses were un-documented still also and quickly add pages for both. I guess we also should add some page below "Nuxt" for our varying starters.
If we could easily support confirmation pages, that would be best. Else I guess we should at least have a working default.
Generally, we are not warning users of not supported stuff, that would be lots of work, would it? Or should we make an exception for that crucial setting and grey-out/disable not supported options? This means, enabling the module takes over all webforms and considers them decoupled. Maybe it would be nice to allow configuring that, but again, complexity, so I think it's ok to keep it simple and make all webforms deocupled once turned on.
> Can it be the site-wide default theme?
That would make regular traditional rendered pages ugly. I'd not do that by default.
> * Possibly auto-enable in default project for forms.
We could do this by adding it to our routes as _theme key - the lupus_Decoupled_forms module has the theme negotiater for switching it built in. If it has a nice fallback to keep the default theme if it's not enabled, we could even set it as default.
Thank you! Additions are good, also all comments have been addressed now. So I think this is ready!
the README already has some good docs to start with: https://github.com/drunomics/nuxtjs-drupal-ce?tab=readme-ov-file#default...
Update on the todos:
* add hosting options
* #3479269: Document default components at website
New todo:
* Add a Drupal introduction page
thanks! please see my comments!
added two new "themes", simply as alternating demo starters. That way we can have multiple gitpod URLs for testing all of them.
Until we have anything better, let's use this.
See https://lupus-decoupled.org/get-started/play-online for a list of frontends, there is one nuxt with shadcn and one nextjs starter.
@glynster: I would love to add https://github.com/StirStudios/stir_nuxt_base there as well. Is it a layer or a full repo now? If a layer, I guess we could just add a small wrapper demo-repo to make it work witih gitpod? I'm opening a dedicated sub-issue for it!
thx! works great and even has solid test coverage! let's merge!
I'd have preferred keeping the code-base tidy when it's unrealistic that anyone is using that yet. Anyway, I'm fine with your proposal to do it proper from the beginning, let's move one with this.
If we don't want to have this message on lupus-decoupled, then we can ship a default CE display for each Core node bundle (and perhaps a CE display for the 'full' view mode too, if we need it). But I guess that's a followup that could wait until we implement proper output for Drupal CMS content types.
I think the message is bad for UX since users opening the screen the first time will have to read it and try to parse/understand what it means for that. Given this, I think for UX we should try hard to avoid this. Also this is not a pattern core introduced anywhere and it'S ususally good to follow core patterns in contrib.
I must say I'm not fully understanding why this message is needed and why it's confusing without. Why is it important whether the configuration you see is acutally saved or not? It should not matter, because either way the same configuration should be applied.
major: if they press the "Save" button without doing anything else, the output will likely change for any view mode that has its own entity view display (because it will be based on the default CE display, from then on).
I cannot follow, could you elaborate what changes? Does core face the same problem?
I still think we need to do what I was writing before:
We'd have to forward the selected langcode. So we must pass the entity language as $langcode when calling this.
Next, the bug might exist in other processors also. When we fix it, we should fix it in all of them.
For 2.x I think it's good enough to fix this with the call-out to the global or even not fix it -> 3.x is the main branch now.
For 3.x we should fix it properly.
i.e. we must extend our API to accept an optional $langcode parameter, so we can pass it on during processing *explicitly*. This is what core does during rendering as well. I guess this means we need to adjust the processors to do so. A new optional $langcode = NULL parameter is no big change, since it keeps BC when the interface was implemented exactly before.