Vienna
Account created on 21 January 2005, about 19 years ago
#

Merge Requests

Recent comments

πŸ‡¦πŸ‡ΉAustria fago Vienna

exciting to see this after soo many years!

I studied the MR and it seems mostly great, I found only one small remark (see above)

πŸ‡¦πŸ‡ΉAustria fago Vienna

I've merged into 1.x

Re-checking on how to solve this, some thoughts:
* We need to have a simple way to wrap a built $form render array into a CustomElement drupal-form
* Would be nice to add a FormRouteEnhancer that automatically adds in CustomElementsFormController if _format is custom_elements
* Then backend form rendering into a CE-enabled API response should be straight forward and progress form submissions in SSR should mostly just work
* Once that is working, JavaScript enhanced form submissions are less a priority for SSR sites, more for SSG sites. That can be handled in a follow-up then.
* As demo forms we should have contact forms and user login forms sub-modules

πŸ‡¦πŸ‡ΉAustria fago Vienna

The proposed resolution
> If they don't have a 'entity' option, then change them to the 'default' front end base URL.
sounds good.

But, I don't think we should touch that part:

if ($absolute || $request->getRequestFormat() != 'custom_elements') {

Generally, I think this check is correct and should not be changed. Yes, maybe there are special-cases when we don't want it to happen, but then we should work them out individually, without touching that general check/logic. That's quite foundational and I don't think we should alter this check now - we want to stabilize and release.

if ($absolute || !$request->attributes->get('lupus_ce_renderer', FALSE)) {
Well, the code tries to determine API-responses, but it might be not 100% right in doing that.

https://git.drupalcode.org/project/lupus_decoupled/-/blob/1.x/modules/lu... is checking for both, what seems correct. Maybe just checking $request->attributes->get('lupus_ce_renderer', FALSE) would be enough, but I'm not sure about that, so that would have to be validated first.

> (I'm trying to write a phpunit test to include here, to enforce that links handled by the Redirect module will stay relative. However I'm having some issues with that at the moment.)

That would be a good improvement, but when hard it might better done as follow-up or added as acceptance-test. The other test-coverage improvemetns seem great already!

But the NULL format is also necessary if we want to prevent redirects specifically implemented through the Redirect module in API output, to be changed from relative to absolute unnecessarily. (Because the redirect module intercepts the REQUEST event, before the lupus_ce_renderer module sets the request format.)

I see. Could we make lupus_ce_renderer module to come earlier? Else, I guess we could simply check for both, to also catch those edge-cases, and call it a day.

πŸ‡¦πŸ‡ΉAustria fago Vienna

thx, I verified this works as it should now really.

πŸ‡¦πŸ‡ΉAustria fago Vienna

This change seems straight forward. PR is good also + thanks for providing demo-video. Merged!

Thank you for your contribtion!

πŸ‡¦πŸ‡ΉAustria fago Vienna

thx, summary is fine. also, let's add the logo.

Somehow I cannot use the UI to merge https://git.drupalcode.org/project/lupus_ce_renderer/-/merge_requests/34 - @lio could you take care of that?

πŸ‡¦πŸ‡ΉAustria fago Vienna

Could we do something like

if (($entity instanceof ContentEntityInterface ||
(class_exists(WebformInterface) && $entity instanceof WebformInterface))

? so we keep the check clean on interfaces, but still do work fine if webform module is not there. It also better documents why we are doing this. (webform)

πŸ‡¦πŸ‡ΉAustria fago Vienna

thx, seems all good and green. thus merged!

πŸ‡¦πŸ‡ΉAustria fago Vienna

thanks, reviewed, green and merged!

πŸ‡¦πŸ‡ΉAustria fago Vienna

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

πŸ‡¦πŸ‡ΉAustria fago Vienna

ok, thanks. that works well, merged then.

πŸ‡¦πŸ‡ΉAustria fago Vienna

I see, makes sense.

TEst coverage improvement is great, however not fully there:

e.g. from test-data

'Frontpage param CE-API trailing slash' => ['/ce-api/?param=1', '', TRUE, '/'],

We should see this resulting into /?param=1 - thus we want to test not only the resulting path but the resulting request-URI really. Could you adust the test-case like this? Then I think this is ready to go.

πŸ‡¦πŸ‡ΉAustria fago Vienna

reviewed + tested, all good now! Thank you.

πŸ‡¦πŸ‡ΉAustria fago Vienna

improving issue title to reflect both are going to work. Is this correct? (if not we must change composer deps)

πŸ‡¦πŸ‡ΉAustria fago Vienna

This looks all good. Which version is used on CI now?
If we need to change our test-dependencies to make it use metatag v2 as well, I think we should do it. Please take a look. Else this seems ready to go!

πŸ‡¦πŸ‡ΉAustria fago Vienna

> if (array_diff(['hasLinkTemplate', 'getTranslationLanguages'], get_class_methods($entity)) &&

This should check for the dedicated interface we need. each method should be provided by a suiting more narrow interface, content-entity-interface is a collection of those. Let's checked for the specific ones with an OR statement so this is a) nicer and b) easier to follow

> This solution works for me, but I've just figured it out that it only adds webform translations if content_translation module is installed which is not optimal. https://git.drupalcode.org/project/lupus_ce_renderer/-/merge_requests/33...

This is not really nice, indeed, but I think good enough. In practice only people having that module installed will want to have these metatags, so let's just keep doing it that way.

πŸ‡¦πŸ‡ΉAustria fago Vienna

it's really great to have a test added here! I do think it's not correctly checking things though, plese take look.

πŸ‡¦πŸ‡ΉAustria fago Vienna

I see, thanks for clarifying.

πŸ‡¦πŸ‡ΉAustria fago Vienna

https://www.drupal.org/node/640498/qa β†’ has the same 2 fails, but shows green tests on 7.4. So let's try that.

Patch seems simple enough, so let's do this.

πŸ‡¦πŸ‡ΉAustria fago Vienna

let's simply get rid of the old service, we are in beta phase and I very very much doubt someone was using that.

πŸ‡¦πŸ‡ΉAustria fago Vienna

* The resulting pipeline does still not pass. Our fix is not fix the pipeline really, but installation with base-paths. Let's open an issue that clearly documents the fix we are doing, fix it there + keep this here open until we can really make it done - with green tests
* Changes seem good, but this is very foundational, so we need to be 100% sure to not cause regressions here. That said, I think this misses test-coverage. Could we add a small unit-test here where we mock $request object, so we could fire it up with all kind of URI combinations and verify it's all correct? (--> in the other issue then)

πŸ‡¦πŸ‡ΉAustria fago Vienna

> Is it reproducible at all on webserver PHP?
Not for me. Only via CLI, with and without drush.

πŸ‡¦πŸ‡ΉAustria fago Vienna

> Edit: I also wish I could know _what_ changed to surface this. I rolled back from an 8.1.26 container to 8.1.25 and it still occurred. I'm really confused.

I can totally second that. We rolled back all changes, made sure images did not change, tried multiple servers, everything. No idea what suddenly caused this still, I'm quite puzzled about that. We've seen the issue to pop up on 01.12 morning, since it's reliably failing on CI.

πŸ‡¦πŸ‡ΉAustria fago Vienna

The previous patch was not correctly committing the transactions. Fixed it, now changes actually persist as the should.

> We're hitting this same order and stack trace! I can only reproduce this on our CI environment. And only when executing Drush commands.

ha, same for us. But it was reproduable with php-cli without drush also + on multiple CI servers.

I was already thinking the problem is probably in the other drivers as well. Let's make sure the mysql works fine, then we can port the fix to all drivers + to 10.2.x. we'll do some more testing with it

πŸ‡¦πŸ‡ΉAustria fago Vienna

Not sure what went wrong with this MR - here a patch file.

πŸ‡¦πŸ‡ΉAustria fago Vienna

ok, strange this did not pop up during testing? Merged.

πŸ‡¦πŸ‡ΉAustria fago Vienna

Seems fine, where is the summary added?

little update in casing:
> Provides out-of-the-box solution for component-oriented Decoupled Drupal with Nuxt as a frontend. More at https://lupus-decoupled.org/

πŸ‡¦πŸ‡ΉAustria fago Vienna

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

πŸ‡¦πŸ‡ΉAustria fago Vienna

Merged, thank you!

πŸ‡¦πŸ‡ΉAustria fago Vienna

> drush config:set custom_elements.settings markup_style vue-3

is what is documented in the README and that or the dfeault is probably what people are using.

web_component is what is working atm, so let's use that and avoid changing working options. Given that we should simply correct the broken update custom_elements_update_8202() in 2.x and 3.x to use the correctly working value web_component.

There is no need to introduce a new update function, everyone who has already run custom_elements_update_8202() worked around the problem already, so let's simply make sure it cannot happy any more in the future.

πŸ‡¦πŸ‡ΉAustria fago Vienna

I've not been suggesting this change, Leo Pitt could you clarify?
But given we are not working on getting a configurable CE-UI in, I think this should be simply one of the options available, then there is no need for a special-case processor in 3.x.

So it would make sense to move this into a custom module for now, and make sure 3.x custom elements UI can be configured to use field formatters. Leo Pitt, would that work for you?

πŸ‡¦πŸ‡ΉAustria fago Vienna

Thx. Generally the changes seem good, but I think the code organization needs little work. See my above remarks.

> The /layout-preview route can always be there when the module is active, I think that makes sense anyway.

So code for that route should be moved into the layout_builder sub-module, it's not related to responsive preview really, but a generally layout-builder preview route which responsive preview then uses.

ad πŸ› FrontendRedirectSubscriber should protect against being hijacked by 'destination' Needs review : not sure, but if it's not popping up again I'd agree to not handle it here

πŸ‡¦πŸ‡ΉAustria fago Vienna

Giving this topic a bit more though, I think we need to consider how forms should be handled progressively first, because that will influence what we need here.

So for progressive form submissions, I think we want to the backend render the forms as usual and just make it so that the form-markup gets shipped into a drupal-form custom element. We pass the form content as nested content slot, and all the regular
attributes as props.

Then, the frontend can easily render the form while controlling necessary attributes like the form action. Usually, by default, we just point the form action against the same route, like drupal does. So a POST to the same route is forwarded by the nuxt-server to the backend, where the form submission is processed as usual. If it results in a redirect with messages, the server will handle this (like we do for every page request). If it results in a reloaded form it can be rendered as part of the page as usual, optionally with messages. So the progressive submission should work quite straight-forward.

Then, the frontend can handle more case, thanks to controlling the form element:
- The frontend can add event handlers for handling form submissions with JavaScript. The JavaScript needs to post the data to the Drupal route of the page and handle the response. We might be able to leverage Nuxt server components elegantly for that, which would re-render server side when updated. We can explore something like a server-only drupal-form-handler component for that, which is used by the wrapping element.
- When doing static generation, the frontend can take care of re-fetching the form and form-build ID as needed. We could provide a composable for that. Then the form is fully working based on JavaScript-enhanced submissions.
- Static generation won't have working progressive form submissions, unless pointed directly to the Drupal backend. If that's good enough the frontend can do so for some forms by making the form action absolute. of course form validation errors would not be handled properly.

So what does that mean for the backend:

  • For progress form submissions, we should support re-rendering the whole page when form data is posted.
  • For handling JavaScript enhanced form submissions, we should support only rendering the form as part of the response. Alternatively, the frontend would have to cut-out the relevant part of the response.

Given that, I don't thank we should add forms with absolute form actions on the backend, but focus on rendering forms within the CE-page-response in a wrapper. That will allow use to make progressive forms work quite easily, for all kind of drupal forms, plus enable us to add JavaScript-enhanced submissions in a following step.

πŸ‡¦πŸ‡ΉAustria fago Vienna

This belongs into lupus_ce_renderer issue queue, which has its own d10 compatible version. Please make sure you are using the most recent one and provide more information then.

πŸ‡¦πŸ‡ΉAustria fago Vienna

Thank you, amazing progress!

Please see my above remarks.

since it might be little confusing, in regard to:
> wondering, would it be hard to make it entity-generic? could be a follow-up, but it would make sense to document what we support then
this is answered by your comment already, to which I replied that one:
> I see. Let's make a setting for this features in the lupus-decoupled-drupal settings screen then? It can be on by default, but there we can also clarify it only works for nodes atm.

So I think main thing that is left to be done here is figuring out to treat various kinds of blocks nicely via the decorator. Then we can add the setting and do more testing!

πŸ‡¦πŸ‡ΉAustria fago Vienna

yep, thanks - let's call it fixed then!

πŸ‡¦πŸ‡ΉAustria fago Vienna

Thank you, merged!

πŸ‡¦πŸ‡ΉAustria fago Vienna

Checking this again, I'm still wondering about that:

So the NativeCustomElementsFormController class is confusing me now - it seems to wrap the form in lupus-form like the regular form-controllers we provide, so what's the point? could we just go with CustomElementsEntityFormController instead?

What makes a native form a native form?

We could modify it inside CustomElementsFormController::getContentResult() but this feels like the wrong place, and is coupling the changes to the form controller, whereas we only really want it in the user_form module.

This seems to be right place to me, so NativeCustomElementsFormController:getContentResult() could make sure the URL is absolute and form submission starts working natively that way. If we could also solve the redirect to be triggered within NativeCustomElementsFormController one could add support for native-forms simply by adding in the right routes with NativeCustomElementsFormController. That would be a quite good DX imho.

But then on the frontend I think we should differ between vue-js handled form submissions and natively working forms which need no special handling at all. What do you think about doing custom elements with the names:
- the native one
- the one rendering into custom-elements, so the frontend has to take care of processing it

πŸ‡¦πŸ‡ΉAustria fago Vienna

Looks all good, needs some testing though still since we have no automated tests for this.

πŸ‡¦πŸ‡ΉAustria fago Vienna

I see, makes sense - but then it's the question whether it's displayed at the page level already, that depends on the page template/component. I think it still makes sense to pass through the information, so the frontend is free to use it or not. Makes sense?

πŸ‡¦πŸ‡ΉAustria fago Vienna

I don't think it makes sense to start special casing various type of blocks and adding individual support for each. Can we find some generic way to allow all-kind of blocks to opt-into rendering correctly?

in particular since we have two place rendering blocks - layout-builder and blocks-layout integration, we have to resolve it without code-duplication.

this is how blocks are handled generically for layout-builder:
https://git.drupalcode.org/project/custom_elements/-/blob/8.x-2.x/src/Pr...

Could we simply do the same here and have both content-entity and views block working?

Still, I think we need to introduce a way to add custom code per-block class that builds the block into custom-elements. So far, we've been doing that by swapping out the block-class for our projects:

/**
 * Implements hook_block_alter().
 *
 * Alter the block plugin definitions for Layout builder's inline blocks.
 */
function ldp_layout_builder_block_alter(&$definitions) {
  foreach ($definitions as $id => $definition) {
    if (strpos($id, 'inline_block:') === 0) {
      $definitions[$id]['class'] = LdpInlineBlock::class;
    }
    if (strpos($id, 'block_content:') === 0) {
      $definitions[$id]['class'] = LdpContentBlock::class;
    }
  }
}

and then this build method:

   * {@inheritdoc}
   */
  public function build() {
    if ($this->getCurrentRequest()->getRequestFormat() == 'custom_elements') {
      $block = $this->getEntity();
      $custom_element = $this->getCustomElementGenerator()
        ->generate($block, $this->configuration['view_mode']);
      return $custom_element->toRenderArray();
    }
    return parent::build();
  }

let's discuss how to do that best!

πŸ‡¦πŸ‡ΉAustria fago Vienna

thanks! I've found one issue with it though, please check!

πŸ‡¦πŸ‡ΉAustria fago Vienna

thank you! That looks generally good, but I think we need to little more cleaning up before merge:

* getBlocks() code-comments need to be updated to reflect the new argument
* I think we should make the new argument $cacheableDependency optional, keep it NULL when omitted and only add cache-dependencies when passed. That way, we can keep up proper BC, in case someone would already use the method (what I doubt though)

πŸ‡¦πŸ‡ΉAustria fago Vienna

To test, the following commands are needed:

ddev composer require drupal/schema_metatag
ddev drush en lupus_decoupled_schema_metatag schema_article schema_web_page -y
// Enable schema_* modules as needed.

This automatically installes schema_metatag v2.5, I guess do to the version of metatag in the codebase. For the time being, that's a good first step, let's test / make compatibility for v3 work as a next step (follow-up issue)

Then one needs to make sure to have a recent page-component, it seems the one in the demo is outdated and missing metatags integration.
I tested it with https://github.com/drunomics/nuxtjs-drupal-ce/blob/2.x/playground/pages/... and it works then.

-> thus, let's merge this and make follow-ups for
* updating the nuxt-demo with latest playground
* add support for schema-metatag v3

πŸ‡¦πŸ‡ΉAustria fago Vienna

ok, I figured the reason is rather simple. We simply forgot to put the integration code into lupus-decoupled... sry!
I'm going to add it now.

πŸ‡¦πŸ‡ΉAustria fago Vienna

let's organize code well and add a lupus_decoupled_responsive_preview sub-module for everything that requires responsive-preview.
we decided it's a good idea to do that for all contrib integrations

πŸ‡¦πŸ‡ΉAustria fago Vienna

Patch looks good, thank you! That should fix it.

We don't need the comment though, it just comments what you see from the code anyway, so it can be removed.

Let's verify this fixes the issue though, i.e. test it.

πŸ‡¦πŸ‡ΉAustria fago Vienna

Update on options:
* Layout builder with Frontend preview via Iframes πŸ“Œ Frontend-rendered layout-builder previews Needs work
* Same Page Preview module integration (full iframe)
* Layout builder + Render custom elements within Drupal Theme (similar to SPA_admin)
* Integrating with Drupal Gutenberg

πŸ‡¦πŸ‡ΉAustria fago Vienna

I tested it on gitpod and was able to reproduce the error. So filing a bug-ticket as sub-task here.

πŸ‡¦πŸ‡ΉAustria fago Vienna

yes, but when people have not done that, we cannot provide an install command one-liner

Problem seems to be that the drupal.org packaging adds module dependencies to composer.json dependencies:

$ composer show drupal/lupus_decoupled -a
name     : drupal/lupus_decoupled
descrip. : Ready-to-go Decoupled Drupal with Nuxt.js!
keywords : Drupal, Decoupled
versions : 1.x-dev, 1.0.0-beta2, 1.0.0-beta1, 1.0.0-alpha4, 1.0.0-alpha3, 1.0.0-alpha2, 1.0.0-alpha1, dev-1.x
type     : drupal-module
license  : GNU General Public License v2.0 or later (GPL-2.0-or-later) (OSI approved) https://spdx.org/licenses/GPL-2.0-or-later.html#licenseText
homepage : https://www.drupal.org/project/lupus_decoupled
source   : [git] https://git.drupalcode.org/project/lupus_decoupled.git 1443c34114e84eb71608f2351f2d2904e04de10c
dist     : []  
names    : drupal/lupus_decoupled

support
source : https://git.drupalcode.org/project/lupus_decoupled

requires
drupal/core ^9 || ^10
drupal/lupus_decoupled_ce_api *
drupal/lupus_decoupled_cors *
drupal/lupus_decoupled_menu *
drunomics/service-utils *
drupal/config_rewrite ^1.5
drupal/custom_elements ^2.2 || 3.x-dev
drupal/lupus_ce_renderer ^2.3.3
drupal/rest_menu_items ^3.0.3
drupal/trusted_redirect ^1.11.0
</code>

But in 
https://git.drupalcode.org/project/lupus_decoupled/-/blob/1.x/composer.json?ref_type=heads
there is no dependency lupus_decoupled_ce_api - so we cannot add the @beta there.

Maybe it works when we add requirements to the modules explicitly. Drupal.org seems to generate meta-packages for sub-modules:
<code>composer show drupal/lupus_decoupled_ce_api -a
name     : drupal/lupus_decoupled_ce_api
descrip. : Provides custom elements API at /ce-api
keywords : 
versions : 1.x-dev, 1.0.0-beta2, 1.0.0-beta1, 1.0.0-alpha4, 1.0.0-alpha3, 1.0.0-alpha2, 1.0.0-alpha1, dev-1.x
type     : metapackage
πŸ‡¦πŸ‡ΉAustria fago Vienna

so it's working with those versions
"drupal/schema_metatag": "2.5.0",
"drupal/metatag": "^1.26.0",

Meanwhile there is https://www.drupal.org/project/schema_metatag β†’ v3, so I'd assume that using that major version breaks things. So we should add compatibility with that.

Could you confirm you are on version3?

πŸ‡¦πŸ‡ΉAustria fago Vienna

Sry for being late to the party here.

The API output posted seems wrong, so I think there might be some incompatibility with recent module updates.
We have this working correctly, e.g. see our website https://drunomics.com/ and the API output https://cp.drunomics.com/api/drunomics-com/

The JSON-LD shoudl be in its own section:

{

      "title": "drunomics | Professionelle Drupal Entwicklung aus Γ–sterreich",
      "messages": [ ],
      "breadcrumbs": [ ],
      "metatags": {
            "meta": [
             ...
            ],
            "link": [
...
            ],
            "jsonld": {
                  "@context": "https://schema.org",
                  "@graph": [
                        {
                              "headline": "drunomics | Professionelle Drupal Entwicklung aus Γ–sterreich",
                              "name": "drunomics | Professionelle Drupal Entwicklung aus Γ–sterreich",
                              "description": "Wir entwickeln hochqualitative und innovative Web Projekte mit dem Content Management System Drupal."
                        },
                        {
                              "@type": "WebPage",
                              "@id": "https://drunomics.com/",
                              "headline": "drunomics | Professionelle Drupal Entwicklung aus Γ–sterreich",
                              "breadcrumb": [ ],
                              "description": "Wir entwickeln hochqualitative und innovative Web Projekte mit dem Content Management System Drupal.",
                              "name": "drunomics | Professionelle Drupal Entwicklung aus Γ–sterreich",
                              "url": "https://drunomics.com/",
                              "datePublished": "2023-03-26T08:49:16+0200",
                              "image": {
                                    "@type": "ImageObject",
                                    "representativeOfPage": "True",
                                    "url": "https://cp.drunomics.com/files/styles/facebook/public/2023-07/drunomics_logo_final.png?itok=claacc7z",
                                    "width": "1200",
                                    "height": "630"
                              }
                        }
                  ]
            }
πŸ‡¦πŸ‡ΉAustria fago Vienna

Thanks! Let's use this issue for the missing documentation. I'd recommend adding it to the linked page on lupus-decoupled.org, then maybe have a point to the page in the README. You could help with that simply by filing a PR against https://github.com/drunomics/lupus-decoupled-website to add it!

For the mentioned problem, it might be a compatibility issue on Drupal 10.1 - let's reproduce it with a gitpod. Could you open a dedicated ticket for the bug? Then let's solve it there. Then we need to work on test-coverage fro this (follow-up)

πŸ‡¦πŸ‡ΉAustria fago Vienna

If possible, the integration with responsive-preview module should be optional, so it just works when the module is active, but it's not hard-dependency. The /layout-preview route can always be there when the module is active, I think that makes sense anyway.

πŸ‡¦πŸ‡ΉAustria fago Vienna

Thank you for the contribution!

Yes, we should load the config as late as possible, but it seems the PR converts one call wrong.
Then, we should avoid loading the config multiple time in a single page load, thus I think we need to introduce some sort of getter to load the config and statically cache it in a local variable.

πŸ‡¦πŸ‡ΉAustria fago Vienna

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

πŸ‡¦πŸ‡ΉAustria fago Vienna

straight forward fix, should clearly use the interface.

πŸ‡¦πŸ‡ΉAustria fago Vienna

Letting this sink, I think the option "Proxy API" is the way go here. For that we need to do https://github.com/drunomics/nuxtjs-drupal-ce/issues/165 and then provide the user-login on the frontend.

πŸ‡¦πŸ‡ΉAustria fago Vienna

@yannickoo: Amazing, thank you! Let me know if you need any further help!

P.S. Please also tag a d10 compatible release, that would be helpful!

πŸ‡¦πŸ‡ΉAustria fago Vienna

reviewed and tested, works good again after applying the patch!
without the patch the add-in-between button are simply gone.

πŸ‡¦πŸ‡ΉAustria fago Vienna

Changes seems fine, the module is rather simple, can we have this merged and a d10 compatible release?

πŸ‡¦πŸ‡ΉAustria fago Vienna

let me try:
well, without the type you cannot re-create the data definition based upon the toArray()
This works well when working with other classes based upon DataDefinition, but apparently not for lists.

I don't see it causing subtle other bugs, still it's not working as it should, thus qualifying it as bug seems right to me.

Production build https://api.contrib.social 0.61.6-2-g546bc20