Hungary
Account created on 28 September 2003, almost 22 years ago
  • Full stack community organizer at Acquia 
#

Merge Requests

More

Recent comments

🇭🇺Hungary Gábor Hojtsy Hungary

Added to the menu structure

🇭🇺Hungary Gábor Hojtsy Hungary

This is caused by 📌 ComponentSourceInterface::submitConfigurationForm and validateConfigurationForm are never called Active apparently because Webform uses Drupal\Core\Entity\Element\EntityAutocomplete which has a validateEntityAutocomplete which is where the mapping happens. That is defined as an #element_validate on the element which is never called. We could potentially mark this as a duplicate of that one but maybe good to keep a use case specific issue around, especially with the "webform" name where people will encounter it.

🇭🇺Hungary Gábor Hojtsy Hungary

Persistent of the field was fixed in 0.6.0, but the problem of the autocomplete not working still stands. That is now down to 🐛 Autocomplete machine name handling does not match to machine name Active which is caused by 📌 ComponentSourceInterface::submitConfigurationForm and validateConfigurationForm are never called Active , so we can consider this one issue fixed.

🇭🇺Hungary Gábor Hojtsy Hungary

Sorry for the noise :D Was not very robust with select default for newly placed blocks. This will avoid an error when you newly place a webform component in XB.

🇭🇺Hungary Gábor Hojtsy Hungary

Also storing the select list patch here for those that need it to make webform work ASAP, although the bug around the autocomplete is in XB not in webform.

🇭🇺Hungary Gábor Hojtsy Hungary

Text looks fine. For reference it would appear once at this place, where I drew the jiggly lines.

🇭🇺Hungary Gábor Hojtsy Hungary

Opened 📌 Add a developer tooling topic to Drupal core Active but that is not where the meat of the description would go based on the structure of the governance there, it would go into the gates definitions, which are d.o docs.

🇭🇺Hungary Gábor Hojtsy Hungary

Ben tells me this is a different issue, so reverted to what it had before I took it over, so this problem is still being tracked. Adding back the reference though to make sure that is not forgotten.

🇭🇺Hungary Gábor Hojtsy Hungary

I'm running into this in 🐛 Autocomplete machine name handling does not match to machine name Active with the webform block.

Webform uses Drupal\Core\Entity\Element\EntityAutocomplete which has a validateEntityAutocomplete() which is where the mapping happens. That is defined as an #element_validate on the element. I tried the proposed fix in #4, that did not seem to help (change anything at all with what it did).

🇭🇺Hungary Gábor Hojtsy Hungary

I tried that, thanks, but that did not seem to change anything.

From what I see src/Element/RenderSafeComponentContainer.php is where the logging comes from which catches all throwables:

  public function renderComponent(array $element): array {
    $context = new RenderContext();
    $element['#children'] = $this->renderer->executeInRenderContext($context, function () use (&$element, $context) {
      try {
        return $this->renderer->render($element['#component']);
      }
      catch (\Throwable $e) {

So I think re-throwing it will still make it end up here and caught wholesale.

🇭🇺Hungary Gábor Hojtsy Hungary

Unfortunately we hit this right away when integrating webform with XB. As explained in the issue summary entering the machine name in the block prop works, but not the autocomplete. Unfortunately it is loaded back as a Title (machine_name) so when/if you edit it you need to remember to edit it back to machine name only.

I opened 🐛 Autocomplete machine name handling does not match to machine name Active for that but is a duplicate.

🇭🇺Hungary Gábor Hojtsy Hungary

BTW the testimonial component has the same problem, you may want to fix it here to be a div for the quote not a p too or open a new issue :) That at least is conceptually easier since the quote is not conceptualised as a "paragraph" :)

Current:

After changing to div:

🇭🇺Hungary Gábor Hojtsy Hungary

The "problem" was that the template has a real HTML paragraph element for this component. Paragraphs can't contain other paragraphs or lists, so the paragraph naturally ends when you enter a list. Then the styling does not apply to that anymore. Once the wrapper is a div, not a paragraph, this is solved. Not semantically very nice but quickly fixed it :D

BEFORE

AFTER (added another list too):

🇭🇺Hungary Gábor Hojtsy Hungary

I think its not an 80% use case module. The general consensus earlier was that it is little maintenance but looks like its not quite true. Quoting the other issue:

One of the biggest challenges with the current Telephone module is that it's just a glorified textfield. It has no option to normalize the telephone number - either for the purpose of validation or formatting.

I don't think its worth doing that investment in core given it is not an 80% use case AFAIS. Removing tag as a product manager.

🇭🇺Hungary Gábor Hojtsy Hungary

Combined patch for xb-demo until later tagged version includes this.

🇭🇺Hungary Gábor Hojtsy Hungary

Based on that updated understanding I proposed an update to the XB error at 🐛 DX: Block plugin full validatability message may be misleading, add concrete next steps Needs review which just got merged. So that explains clearly that we need to add FullyValidatable after all.

🇭🇺Hungary Gábor Hojtsy Hungary

Ok I was misguided. FullyValidatable does not mean what the name suggested to me it meant. That config inspector says a tree is entirely validatable does not mean it is FullyValidatable, because FullyValidatable is a strict validation opt-in flag, not a status indication of the validatability of a tree. So it is something that must be explicitly added and is not deduced. In short it means that all values are required by default under the key where it is specified and all keys of a mapping must be present.

More at https://www.drupal.org/node/3404425

So yeah we do need to opt in to the strictly validated for webform blocks to show up in XB, sorry for the prior confusion.

🇭🇺Hungary Gábor Hojtsy Hungary

Isn't FullyValidatable deduced from the data structure being fully validatable? We would only need to specify it if we want to work around parts of the data structure not being validatable and we want to force it?

While XB does tell us the webform block config is not validatable, it does not tell us why. If you place a webform block and check it with config inspector, nothing under its setting is impossible to validate. (The top level of webform's settings says validatable).

In @wim.leers words:

XB doesn’t use Block config entities
It uses BlockPluginInterfaces aka block plugins
So XB stores a subset of what Block config entities store: only `

settings:
  type: block.settings.[%parent.plugin]

aka block.block.*:settings
See \Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\BlockComponent::checkRequirements() :

$data_definition = $this->typedConfigManager->createFromNameAndData('block.settings.' . $block->getPluginId(), $settings);

But this part is already entirely validatable as proven by config inspector (see screenshot). If we need to add FullyValidatable manually to config trees that are already fully validatable, that sounds like a lot of extra things to do, and it will potentially lead to problems if new not validatable keys are introduced accidentally, which the workaround will paint over and hide.

🇭🇺Hungary Gábor Hojtsy Hungary

Made diff permanent for composer patching until a release of webform includes the fix.

🇭🇺Hungary Gábor Hojtsy Hungary

Re #13, basically this was missing in my MR compared to #13. I don't think this should be committed as-is but this does make metatag work with XB and not emit any scary widgets :)

      $form['metatags']['widget'][0]['advanced']['#access'] = FALSE;

Also adding the current diff file for composer patching purposes.

🇭🇺Hungary Gábor Hojtsy Hungary

This is intended to cause a fatal error, that is what is being tested that Upgrade Status does not collapse on scanning a module with a PHP fatal error in it :) So not sure what you mean here? If we would comment out that line it would not cause the fatal error that we need.

🇭🇺Hungary Gábor Hojtsy Hungary

That said, if people want to invest time into this, I don't think it is a problem at all to change this. So removing the needs product manager tag.

🇭🇺Hungary Gábor Hojtsy Hungary

Drupal CMS is the primary thing promoted now on drupal.org. From DrupalCon Vienna (2025 October) onwards that will include Umami-like things (known as site templates) that are built-out demo sites of sorts but more complete and include contributed projects and Experience Builder. So the age of Umami is pretty much over at that point as it is unfortunately. I think ideally it is moved to contrib and could flourish as a site template built on XB (keeping the recipes, photos and all that people took so much precious time building). In that light I think investing time into making the core Umami demo better is not necessarily the best thing to do.

There is 📌 Make Umami a testing profile (not shown to users) in preparation of moving to contrib Active and 📌 Deprecate and remove the Umami demonstration profile from core, make it a contributed site template Active .

🇭🇺Hungary Gábor Hojtsy Hungary

Updated version of #21 for composer patching :)

🇭🇺Hungary Gábor Hojtsy Hungary

Makes sense. Ideally it would not even be needed if the autocomplete would properly hold the value in XB. Rolled that part back in interest of moving this forward :) I think the block validatability changes are noncontroversial :)

🇭🇺Hungary Gábor Hojtsy Hungary

I meant to move it to RTBC as the core committer looking at this could consult the security team too :)

🇭🇺Hungary Gábor Hojtsy Hungary

I think the UX reviews are properly resolved by simplifying the form. The main thing I see is this needs a proper security team review. It makes confusing two executable paths very easy on the UI and thus makes it easier to run arbitrary executables via Drupal. In the UX review it was raised that some validation could stop it from saving arbitrary executables as valid values, but that still does not rule out excuting things that were placed on the server. I imagine that may be out of scope of the Drupal security coverage, but that is best said by the security team. Otherwise I think this looks good.

🇭🇺Hungary Gábor Hojtsy Hungary

gábor hojtsy made their first commit to this issue’s fork.

🇭🇺Hungary Gábor Hojtsy Hungary

Ok this now makes phpstan and cspell happy and makes a dent in phpcs :) Good enough for me.

🇭🇺Hungary Gábor Hojtsy Hungary

@godotislate: I don't believe that is a problem, we are testing whether on form submission the entity gets the language, and it appears it does not (likely the third party setting is not saved).

🇭🇺Hungary Gábor Hojtsy Hungary

Can you put this into an MR? That way tests would run on it too :)

🇭🇺Hungary Gábor Hojtsy Hungary

The remaining fails are around #edit-seo-settings #edit-metatags-0-basic-title of course. I still did not find where this one comes from metatag, but access false-ing the parent does make this inaccessible apparently.

Would be good to get a clear answer from the XB team whether the field should stay and further altering should be put into place (as in #13 but that does not list all the items, see #14) or the field should be removed (which does sound like would be the cleaner solution IMHO).

🇭🇺Hungary Gábor Hojtsy Hungary

I understand that @damienmckenna. I don't have an opinion either way, so I personally leave considering that to @lauriii and/or other code owners in this area of XB :) My main aim is to make metatag and XB work peacefully together, I don't have architectural opinions about this, trying to help with whatever is needed :)

🇭🇺Hungary Gábor Hojtsy Hungary

Very complete suggestion, thanks! From my visual review, it looks fine, however I don't use config split :) Can you get anyone else review / try it? It looks like it would not have negative side effects at least, but would be good to have another set of eyes/hands on it.

🇭🇺Hungary Gábor Hojtsy Hungary

Due to a mismatch in when I checked the credit, the commit credit did not land with @penyaskito and @diegors unfortunately, sorry. The issue credit was properly set though.

🇭🇺Hungary Gábor Hojtsy Hungary

I'm not interested, but it is open source of course, so you can create other versions yourself.

🇭🇺Hungary Gábor Hojtsy Hungary

gábor hojtsy made their first commit to this issue’s fork.

🇭🇺Hungary Gábor Hojtsy Hungary

Most of the calls changed are in static methods, those don't have a $this. Did you actually try if the change worked? Screenshot of it applying cleanly does not prove it works.

🇭🇺Hungary Gábor Hojtsy Hungary

None of the fruits seemed low enough so released this with one other commit I already had :D https://www.drupal.org/project/upgrade_status/releases/4.3.8 Thanks again!

🇭🇺Hungary Gábor Hojtsy Hungary

Thanks all! I get the irony! :) Hope this helps. Will do a release too later today, but will check first if there are other low hanging fruits to include too.

🇭🇺Hungary Gábor Hojtsy Hungary

gábor hojtsy made their first commit to this issue’s fork.

🇭🇺Hungary Gábor Hojtsy Hungary

Had a zoom meeting with Lauri where I showed him the current state. XB has an opinion on what basic SEO fields should be shown and implements all of them as base fields (except the SEO title). So the metatag field is mostly used to wire in the token replacements as shown above in #17. I understand this could be in global config for this entity type. Lauri told me that keeping the field would still be better as it would give the option to create a custom, simpler widget for more complicated SEO settings in the future. I think the field could still be added then, but my aim is to solve metatags not working with XB, not to argue about architecture :)

So attaching a few lines of added code that alters out the metatag section of the form at the existing place where it moves the SEO title field. This works for me, although I don't have the SEO title field somehow in metatag 2.1.1 that I have, so not sure if that would be affected by the #access even though it is moved out of the group. I think for this stop-gap issue that is the remaining question.

🇭🇺Hungary Gábor Hojtsy Hungary

Fails are unrelated to this MR, so moving to Needs review. I think it fulfills what it was meant to do. Webform maintainer decision is needed as to whether the block dropdown makes sense compared to the autocomplete. I think its a better user experience, but could indeed get unwieldy if someone has a loooooooot of forms.

🇭🇺Hungary Gábor Hojtsy Hungary

Let's keep the scope of showing up as dynamic component which is already achieved :)

🇭🇺Hungary Gábor Hojtsy Hungary

Updated issue summary with current problem list and made title more generic given that we have shifted the scope a bit in this issue already :D

🇭🇺Hungary Gábor Hojtsy Hungary

Roy is a legend! Your move away to other life things is very much understandable but you gave so much to Drupal. I will forever remember the drawing meetings where you turned your camera on your piece of paper are we conceptualised things on the spot.

🇭🇺Hungary Gábor Hojtsy Hungary

https://github.com/mdlutz24/drupal-meeting-parser is the chrome extension to save the slack meetings nicely. If there are any bugs, happy to help commit fixes as one of the maintainers :)

🇭🇺Hungary Gábor Hojtsy Hungary

Note that DrupalCon itself provides per speaker social share images, I'll let them know of this issue and try to figure out where that is at, if done already or not, etc. Those could also be used :) I don't think those are the only ones to use exclusively but they will help.

Also there are more AI sessions than listed and also a workshop :) Maybe a decision flow graph kind of thing could also be useful like the one I created back in the day for the contribution events. This one :D

🇭🇺Hungary Gábor Hojtsy Hungary

Also Adam Hoenich may know if it was intended that the recipe name was not pulled dynamically but rather an alternate name was possible to be provided here. That may be a loss of a feature to not allow that anymore. That said it would be harder to translate it then :D

🇭🇺Hungary Gábor Hojtsy Hungary

Is this an API that is being changed or is not? Looks like it is defined by recipe_installer_kit :)

Is this issue for discussion? I think the ultimate MRs will be against recipe_installer_kit and drupal_cms repos?

🇭🇺Hungary Gábor Hojtsy Hungary

I agree with @longwave's summary from #15, removing the product tag as a Drupal core product manager.

🇭🇺Hungary Gábor Hojtsy Hungary

New MR needs to be merged to avoid the (not different!) whitescreen that my previous one introduced.

🇭🇺Hungary Gábor Hojtsy Hungary

Expanding the scope a bit, since this could be about the initial working version of it which seems to not be too far off.

🇭🇺Hungary Gábor Hojtsy Hungary

@damienmckenna: are these mappings possible to reproduce in a global config somehow? I think the reason for the field is to use these mappings for xb metadata (unless explicit settings otherwise).

          ->setDefaultValue(Json::encode([
            'title' => '[xb_page:title] | [site:name]',
            'description' => '[xb_page:description]',
            'canonical_url' => '[xb_page:url]',
            // @see https://stackoverflow.com/a/19274942
            'image_src' => '[xb_page:image:entity:field_media_image:entity:url]',
          ]))
🇭🇺Hungary Gábor Hojtsy Hungary

Woah I figured out that the error in layout was due to the demo design system attempting to use Paragraphs even if its not even installed on the site ONLY if there was a webform :D

It does render now! BUT the selected webform value still does not persist.

🇭🇺Hungary Gábor Hojtsy Hungary

@gxleano suggested to change the widget to a dropdown instead of an autocomplete. That behaves the same. I can even save it but then it says the tree is broken (and even the undo in the dialog does not work).

🇭🇺Hungary Gábor Hojtsy Hungary

When locale is enabled, does the remote translation checking / downloading happen automatically too? I fully agree that this message in the installer is distracting, superfluous and should be removed. It is unfortunate if that also means that there is no guidance once the module is enabled where to take the next step, unless it is already taken for you. This is a problem for other modules as well, so this was/is a one-off solution I think after all but unless it happens automatically as well, I think we are leaving users in the middle of nowhere with this.

🇭🇺Hungary Gábor Hojtsy Hungary

Can we somehow add back the feedback when an individual user action (eg. running translation updates or importing .po file) is done? I think with this MR if you do a manual action that affects your translations, you may not be getting any feedback?

I do agree the same feedback in the installer is useless, superfluous and distracting :)

🇭🇺Hungary Gábor Hojtsy Hungary

Adding related issue in 🐛 Autosave publish process does not acknowledge pathauto deactivation Postponed that may cause the checkbox to not work. Even if that would work I think "metatag" working fine is not quite leaving every single field as-is in the sidebar, so lacking further product direction I think leaving all but the XB base field provided (image / description) SEO stuff out for now would be much better. Otherwise (even if the checkboxes then work fine) enabling metatag throws this massive amount of form elements, most of which are quite confusing and super advanced.

That said, until the checkboxes get fixed we do need a workaround that keeps metatag working IMHO.

Production build 0.71.5 2024