Vienna
Account created on 21 January 2005, almost 20 years ago
#

Merge Requests

More

Recent comments

🇦🇹Austria fago Vienna

thx! works great and even has solid test coverage! let's merge!

🇦🇹Austria fago Vienna

fago made their first commit to this issue’s fork.

🇦🇹Austria fago Vienna

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.

🇦🇹Austria fago Vienna

fago made their first commit to this issue’s fork.

🇦🇹Austria fago Vienna

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?

🇦🇹Austria fago Vienna

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.

🇦🇹Austria fago Vienna

there was indeed a regression for this at some point, but that has been solved. Could you report if the problem is still there with latest releases?

🇦🇹Austria fago Vienna

this seems to miss some essential test - the actual login. I think we should add that.
also, this is only about user-forms atm, what is fine. Let's make one issue per test we add, to keep a better overview.

🇦🇹Austria fago Vienna

Let's mark this is as META and only set the issues with acutal code for needs-review.

🇦🇹Austria fago Vienna

thx, all great! I've improved the README slightly to also link to https://lupus-decoupled.org/advanced-topics/block-layout

🇦🇹Austria fago Vienna

fago made their first commit to this issue’s fork.

🇦🇹Austria fago Vienna

thanks! Looks all good, merged.

🇦🇹Austria fago Vienna

fago made their first commit to this issue’s fork.

🇦🇹Austria fago Vienna

re-checking on the solution, I think this is the right way and solves the issue. thus, moving on - merged!

🇦🇹Austria fago Vienna

unfortunately the gitpod link does not work for lupus-decoupled branches still, see https://www.drupal.org/project/lupus_decoupled/issues/3347260#comment-15... 🐛 Gitpod branch checkouts always use 1.x-dev Active

anyway, I checked the code out manually there and tested it at the gitpod environment also. works as it should!

🇦🇹Austria fago Vienna

the DP_ISSUE_FORK variable is missing from our link. thus testing it with a link like this:

https://gitpod.io/new/#DP_PROJECT_NAME=lupus_decoupled,DP_ISSUE_FORK=lupus_decoupled-3485563,DP_ISSUE_BRANCH=3485563-improve-frontend-login,DP_PROJECT_TYPE=project_module,DP_MODULE_VERSION=1.x,DP_PATCH_FILE=,FRONTEND_REPOSITORY=https%3A%2F%2Fgithub.com%2Fdrunomics%2Flupus-decoupled-nuxt3-demo,CUSTOM_ELEMENTS_VERSION=3.*,DP_INSTALL_PROFILE=standard/https://github.com/drunomics/lupus-decoupled-project/tree/main

still, it is ignored, it just takes the 1.x from composer.

🇦🇹Austria fago Vienna

I figured it's best to create a new setting for that and implemented this.
I also improved in-code comments for the existing options, such that there is a better overview of what they actually do. During implementing I realized we need to keep paths, not routes. Since in the path processor, the route name is not available.

The MR adds /user to and the user account page to the "keep frontend" paths, that way a user login in the frontend keeps you in the frontend.

🇦🇹Austria fago Vienna

Works all good! Thus, I added the patch to the project template, that makes the user logout work.

Given that, this works! The redirection logic needs love, but this can be optimized in a followup. Thus both PRs merged!

🇦🇹Austria fago Vienna

@glynster - wow, that's amazing!!

a few questions and thoughts:
- What dependencies / requirements does it have for testing it? Could it be just run with the gitpod demo link and it mostly would work?
- component style - I wonder why you are moving the types out of the components. Wouldn't it be easier to see what's going on if all is within the component, as in https://github.com/drunomics/lupus-decoupled-nuxt3-demo/blob/main/compon...
- we recently moved the best-practice suggestion to just stay with kebap-case for custom elements, so the 1:1 mapping is easier to see. e.g. see https://github.com/drunomics/lupus-decoupled-nuxt3-demo/blob/main/compon.... Any thoughts/concerns on adopting this?
- sitemap, interesting - we just use simple_sitemap module with a suitingly configured base-url + proxy it through on the frontend. Any thoughts on pros/cons of both approaches? I guess we should open a dedicated issue for it!
- the paragraphs seem great! I guess it would be ideal to pair it with a drupal-recipe which adds the necessary paragraph types. For a theme, that seems a bit out of scope though. So maybe the theme would instead just provide a set of optional components, what in the end, they are. So maybe it's a matter of simply documenting which components are there for optional use and maybe have a way to preview them?

> Adds a Vite hook so Nuxt dev tools work locally with DDEV.
I guess we should add this to the nux3 demo also!

🇦🇹Austria fago Vienna

Sounds like a good idea. But I'm not sure what it would entail exactly. As I understand the module adds the data model in the Drupal backend. What kind of integration for next.js/nuxt.js is needed then?

Schema Metatag module is already supported, as long the Schema.org blueprint module is using that, it should just work. Could you elaborate what kind of integation you have in mind, please?

🇦🇹Austria fago Vienna

@usernamee: please don't re-open issues that are fixed for small follow-ups, open follow-ups instead. anyway, thanks for the MR, merged!

🇦🇹Austria fago Vienna

ended up patching core in 🐛 Menu APIs provide invalid CSRF tokens Active . With that I've seen the link working, so testing it now.

🇦🇹Austria fago Vienna

I think this is not important, since generally it's better to make CSRF-token protected links work properly. The confirm-form we could make work, but including the redirect to the form, it's gonna be some additional work that might not be worth it.

🇦🇹Austria fago Vienna

It's probably safe to assume that every non-html request is not going to use the render sytem and thus a placeholder should not be created. Not perfect, but a better check than it was before.

This works for rest_menu_items and for core-linkset endpoint once it's correctly specified as json. Patch attached. Will work on a test also.

🇦🇹Austria fago Vienna

tested it. it does not - it faces the same problem. Let's open a core issue!

🇦🇹Austria fago Vienna

so, re-using the pre-exiting url object does not solve it either. Anyway, Drupal core generates the token with a placeholder, but since we are not rendering obviously placeholders are not replaced. :-( I wonder how this is solved for the drupal core menu linkset api.

🇦🇹Austria fago Vienna

to reproduce, run this with drush php

> \Drupal\Core\Url::fromUri('internal:/user/logout')->toString();
= "/user/logout?token=fzL0Ox4jS6qafdt6gzGzjWGb_hsR6kJ8L8E0D4hC5Mo"

compare the token with the right token, it's wrong, it's the placeholder value. So seems this is triggered by a core bug.

However, additionally rest_menu_items has a bug since it calls > \Drupal\Core\Url::fromUri('internal:/user/logout')->toString(TRUE); but throws the resulting bubbleablemetadata away. By throwing it away, the placeholders won't be replaced.

🇦🇹Austria fago Vienna

I tracked it down: the problem is the wrong token, is not a token, it's a placeholder, which ought to be replaced by the renderer.

See the logic of RouteProcessorCsrf:

      // Adding this to the parameters means it will get merged into the query
      // string when the route is compiled.
      if (!$bubbleable_metadata) {
        $parameters['token'] = $this->csrfToken->get($path);
      }
      else {
        // Generate a placeholder and a render array to replace it.
        $placeholder = Crypt::hashBase64($path);
        $placeholder_render_array = [
          '#lazy_builder' => ['route_processor_csrf:renderPlaceholderCsrfToken', [$path]],
        ];

        // Instead of setting an actual CSRF token as the query string, we set
        // the placeholder, which will be replaced at the very last moment. This
        // ensures links with CSRF tokens don't break cacheability.
        $parameters['token'] = $placeholder;
        $bubbleable_metadata->addAttachments(['placeholders' => [$placeholder => $placeholder_render_array]]);
      }

\Drupal\Core\Render\MetadataBubblingUrlGenerator::generateFromRoute() seems to activate this logic always, even when the Url is generated with $url->toString(FALSE).

🇦🇹Austria fago Vienna

unfortunately the logout link token has some issues also, see 🐛 Wrong user logout CSRF token Active

🇦🇹Austria fago Vienna

ok, worked over it and fixed a couple of things. the token was missing from the link, what I fixed. Still, the login link works now, but the logout link dose not work even when the token is used. It seems the token provided by the menu api is somehow not valid. investigating

🇦🇹Austria fago Vienna

it seems we already have a FRONTEND_BRANCH variable, but it was not fully working yet. So fixed it. https://github.com/drunomics/lupus-decoupled-project/pull/13/files

Test-Link
https://gitpod.io/new/#DP_PROJECT_NAME=lupus_decoupled,DP_ISSUE_BRANCH=1.x,DP_PROJECT_TYPE=project_module,DP_MODULE_VERSION=1.x,DP_CORE_VERSION=10.3,DP_PATCH_FILE=,FRONTEND_REPOSITORY=https%3A%2F%2Fgithub.com%2Fdrunomics%2Flupus-decoupled-nuxt3-demo,FRONTEND_BRANCH=feature%2FLDP-2606,CUSTOM_ELEMENTS_VERSION=3.*,DP_INSTALL_PROFILE=standard/https://github.com/drunomics/lupus-decoupled-project/tree/feature/frontend-branch

🇦🇹Austria fago Vienna

I read through the docs, all good! merged!

🇦🇹Austria fago Vienna

As posted at 🐛 User logout confirmation form is not supported (d10.3+) Active we can workaround the issue with the user logout route by simply showing the content of the "account" menu. Thus, instead of making a request to check the user-login state, we could simply make a request to load the "account" menu and it will give us the right links for login + logout already!

One small glitch: once logged in, it will also include the link to "my account" which redirects to the backend atm. While this is not very nice, I think it's ok and good enough. We can open a follow-up for improving the /user route redirection in the backend.

🇦🇹Austria fago Vienna

I think this is not really a bug now, because there are ways to work around it.

I came up with a simple one. Use the account-menu, which at least in standard profile, is there and provides us with working login/logout links. The logout link already contains the right token. api response for testing: /api/menu_items/account

🇦🇹Austria fago Vienna

I guess best would be a nuxt-layer, since it would take care of dependencies + registering the components with a low-prio OR providing some-sort of install hooks the copies over components automatically during install. thoughts?

🇦🇹Austria fago Vienna

I agree, this would be a great option to have as an option. I guess it would make sense to build it already using the new nuxt-ui alpha version.

However, before we do that, let's clarify how do we build this starter-kits?

Questions I'm wondering about:
* Should it work like a starter-kit, or like a nuxt-module, so you can update later on? I guess starter kits.
* Should it be a nuxt-layer you pull in once? Or just a another repo like the nuxt3 demo which has a couple of components added? The repo seems great, but had the disadvantage that we need to keep it maintained and pull in nuxt updates etc for people to have updated components still..
* Where should we host them? github vs drupal gitlab. I think it would be preferable to keep it on drupal,org, but until Allow GPL-Compatible Licensing for Non-Derivative Code and Drupal.org General Projects Active is fixed, I think we cannot really use drupal gitlab. Thus github and some sort of of maintained registry page (at our doc website) like it's done for nuxt modules would probably work also.

🇦🇹Austria fago Vienna

added MR to enable it and other missing modules. thus the module is now part of the setup.

🇦🇹Austria fago Vienna

great! thank you, merged.

🇦🇹Austria fago Vienna

fago made their first commit to this issue’s fork.

🇦🇹Austria fago Vienna

I merged this and fixed the gitpod setup to work gain, thus closing.

🇦🇹Austria fago Vienna

Status:
I worked over the docs during last weeks and I'm almost done. However, a few pages still need work - so adding todos for them.

todos:
* frontend: custom routes update
* community: ddd23 links + ddd23 page
* work over "getting started"
* add hosting options
* 📌 Document default components at website Active

🇦🇹Austria fago Vienna

I was trying to to test this, but actually failed logging out.

I figured there is an issue with logouts. It used to work before, so probably https://www.drupal.org/node/2822514 caused it.

When the logout is not working, you stay logged in, and the /user/login route shows access denied. Maybe this is what is happening here?
Please try to reproduce with the gitpod online demo and provide reproduction steps.

🇦🇹Austria fago Vienna

Yes, that makes sense to fix. But I think it's not right atm.

When another language, not the current content language, is shown, defaulting to the current content language is not what you want. 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.
In 3.x, we should make sure our formatters do that correctly. Since 3.x is our main development branch, we should first develop it for 3.x and then backport in 2.x

🇦🇹Austria fago Vienna

sounds great, good idea! only comment I have is that we should put the generators in the right modules, i.e. custom elements stuff into custom elements.

🇦🇹Austria fago Vienna

Chiming in here to add some background why GPL is a no-go for many/most Javascript frameworks:

The problem with React/Vue/.. any modern JS frontend is that the templates are JavaScript and transfered to client AND get bundled together typically by route or even by app. Thus any code, or components which end-up in the client-side bundle can easily "infected" by GPL as soon as some GPL library is somewhere used in the app and ends up in the client-side bundle. That would make the whole client-side JS app GPL, what is a no-go for most businesses and licenses troubles people do not want to get into (including myself). That's why in the JS frontend world permissive licenses, like MIT, are standard.

Thus, for decoupled projects, like https://www.drupal.org/project/lupus_decoupled hosting of frontend code requires MIT. Currently that forced us to move all frontend things to github. We'd love to have things like decoupled themes on drupal.org in the future, but the license is a showstopper here atm.

> I don't think that Experience Builder is currently in violation of current d.o. policies around this. We are using MIT licensed code (e.g., React), but that doesn't prevent the Experience Builder module as a whole being licensed under GPL.

While this is not a problem for experience builder per se, it might become a problem if its used for react based frontends in the future. When Drupal provides JS/react-code, I assume it typically woudl be its own bundle, and thus not a problem. However, people might want to include this react code in some decoupled react frontends in the future, for which the GPL license woudl be a blocker again. In order to avoid blocking future integrations like this, I'd suggest to pick a more permissive license there for JS-components/libraries also.

🇦🇹Austria fago Vienna

fixed links and generally slightly improved the docs

🇦🇹Austria fago Vienna

MR is good and all necessary, thus merging and creating a new release.

🇦🇹Austria fago Vienna

re-titling since this got manual testing now

🇦🇹Austria fago Vienna

fago made their first commit to this issue’s fork.

🇦🇹Austria fago Vienna

This seems fine indeed. Let's finally move on here ;-)

🇦🇹Austria fago Vienna

ad #11: yes, it's driving the replacments, see https://git.drupalcode.org/project/social-share/-/blob/8.x-2.x/src/Socia...

Thank you, merging!

🇦🇹Austria fago Vienna

fago made their first commit to this issue’s fork.

Production build 0.71.5 2024