Account created on 19 July 2012, about 13 years ago
#

Merge Requests

More

Recent comments

🇧🇪Belgium herved

+1 on #19
As stated in #7, the current MR seems to break/ignore what the parent issue meant to achieve, which is why I suggested a simple revert for now until a proper solution is found.

🇧🇪Belgium herved

I'm currently on 5.2.0-beta3 with the fix from #9 in my main composer.json, no more issues since then.
But I tested the fix on current dev with procedure from #12 and can confirm it does solve the issue.
So +1 for me.

🇧🇪Belgium herved

Left a small non-blocking comment.
Works perfectly on my side — Many thanks for the excellent support, and thanks to the entire team behind the UI initiative!

🇧🇪Belgium herved

I managed to reproduce on a clean setup! though using xdebug to manipulate requests order.

- Install clean ui_suite_bootstrap with ddev (enable modules + theme)
- Load 1 tab with homepage and another tab with the first aggregated CSS URL on the homepage (sites/default/files/css/...)
- ddev drush cr && ddev xdebug on (clears cache, starts xdebug and clears APCu cache)
- Place a breakpoint on first line in \Composer\Autoload\ClassLoader::findFile with condition: $class === 'Drupal\ui_suite_bootstrap\HookHandler\LibraryInfoAlter'
- Now run 2 concurrent requests:
-- First load the homepage, let it hit the breakpoint and pause.
-- Then load the CSS URL page, let it hit the breakpoint as well, step over a bit and notice that it cannot find the class and will store false in apcu_add() then let it resume.
- At this point the apcu cache is corrupted. All pages are throwing "InvalidArgumentException: Class "Drupal\ui_suite_bootstrap\HookHandler\LibraryInfoAlter" does not exist. in Drupal\Core\DependencyInjection\ClassResolver->getInstanceFromDefinition() (line 32 of core/lib/Drupal/Core/DependencyInjection/ClassResolver.php).
"
The only way to recover is to clear APCu (e.g. via ddev restart or ddev xdebug toggle)

Maybe there are easier ways to trigger this, but this is so far one reliable way I found.
I didn't dig much further but I suggest to just register the namespace in composer as per #9.
Also, subthemes (and so starterkits) may have to do the same if they also use the same "class-based hooks".

🇧🇪Belgium herved

So far I was not able to reproduce on a clean ui_suite_bootstrap ddev setup, sadly.
But it looks like some race condition.
I could only reproduce it on my project and it happens intermittently. This seems to involve the main request + aggregated assets (CSS/JS) requests (needs aggregation enabled).

When that happens, \Composer\Autoload\ClassLoader::findFile gets called on $class Drupal\ui_suite_bootstrap\HookHandler\LibraryInfoAlter while somehow themes are not yet psr4-registered ($this->prefixDirsPsr4 doesn't contain any themes namespaces), so findFileWithExtension returns false and gets stored in apcu_add(). At this point, the only way to recover is to restart ddev or the web server which I assume clears the apcu cache.
Here is a stacktrace and method I used: https://gist.github.com/vever001/009e4163eb6d27cab26a2b5a0df41a62

I wonder precisely why they are not psr4-registered yet in such cases, because IIUC this should happen earlier in the request, in ThemeHandler::addTheme called very early on KernelEvents::REQUEST.

🇧🇪Belgium herved

Update: registering the namespace manually in composer.json does seem to work.
Still a workaround of course, but better than the proposed patch.

"autoload": {
    "psr-4": {
        "Drupal\\ui_suite_bootstrap\\": "web/themes/contrib/ui_suite_bootstrap/src",
        "Drupal\\my_subtheme\\": "web/themes/custom/my_subtheme/src"
    }
},

The one for my_subtheme is only needed if you implement hook classes similar to ui_suite_bootstrap obviously.
Make sure to composer dump-autoload after that.

🇧🇪Belgium herved

Hi, just to report that we're also facing this issue with DDEV. I think the issue is valid, but I haven’t yet investigated what’s going on exactly.

Indeed, themes don’t get PSR-4 autoloading, but these classes are invoked using ClassResolver::getInstanceFromDefinition, so should get loaded if not already?
Also, I wonder if this is specific to APCu — does disabling APCu resolve it? Just to confirm.

I don't know much about autoloading but maybe registering the namespace in composer autoload might be a better workaround for now?
I'll do some testing/investigation and update.

🇧🇪Belgium herved

This issue can be reproduced very easily with a multiple-cardinality link field, as stated in the IS.
I added steps for demo_umami.

The description added by the link module is printed in core/themes/claro/templates/form-element.html.twig:59 as description.content
🐛 Prefix is shown before description Active is a different issue and relates to the field prefix in form-element.html.twig. It doesn't solve it.

🇧🇪Belgium herved

MR rebased, attaching diff for composer

🇧🇪Belgium herved

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

🇧🇪Belgium herved

I had a stab at this again but it still needs some work it seems.

🇧🇪Belgium herved

We ran into this issue after switching to layout_builder on a project.
It’s made worse by the fact that the tempstore is auto-saved whenever you visit a layout builder display (see Drupal\layout_builder\EventSubscriber\PrepareLayout::onPrepareLayout), unlike views for example. Those expire after a week. So when working on displays, you almost always end up with tempstore entries locally, leading to the issues below.

This really feels like a bug for two reasons:
1. It causes confusion, since changes are not visible on the layout builder display pages after a config:import
2. Deleting a computed / extra field / layout etc, then accessing the layout builder display causes a fatal error because the corresponding plugin cannot be found—and layout builder cannot recover.

Here is a possible solution, crude but effective:

/**
 * Implements hook_cache_flush().
 */
#[Hook('cache_flush')]
public function cacheFlush(): void {
  // Clear layout_builder defaults section storage tempstore on cache flush.
  // @see https://www.drupal.org/i/3310897
  \Drupal::database()
    ->delete('key_value_expire')
    ->condition('collection', 'tempstore.shared.layout_builder.section_storage.defaults')
    ->execute();
}
🇧🇪Belgium herved

I didn't dig much further tbh because if I'm not mistaken the report is the same as 3.0.x https://git.drupalcode.org/project/facets/-/pipelines/582244
So this is probably out of scope here and deserves its own issue?
Thanks for looking into it.

🇧🇪Belgium herved

There is another issue, several processor schemas are not defined/missing after 🐛 Config schema fix Active :
- hide_active_items_processor
- hide_1_result_facet
- list_item
- show_only_deepest_level_items_processor
- translate_entity_aggregated_fields
- uid_to_username_callback

Should we extend the scope here?

🇧🇪Belgium herved

#14 & #17, no it should not, those schema definitions should go in the module where the plugin is defined, i.e. facets_summary. These can be used without facets_exposed_filters.
Follow up created here: 🐛 Several schema definitions in wrong sub-modules Active

🇧🇪Belgium herved

herved changed the visibility of the branch 3474018-regression-class-is to hidden.

🇧🇪Belgium herved

PS: the ddev setup is outdated and the pipeline on 3.x is currently broken, out of scope here

🇧🇪Belgium herved

It looks like 🐛 Assets paths in CSS no longer rewritten when aggregation is enabled Active was committed recently.

I think all the code and tests added from both 🐛 Website error Exception: "Only file CSS assets can be optimized" Active and 🐛 Assets paths in CSS no longer rewritten when aggregation is enabled Active are not needed could be reverted if we consider this change here.
For now I will only include here the minimal changes.

🇧🇪Belgium herved

Thanks @saurabhkanva, I created a MR from patch #2 and made some minor changes.

🇧🇪Belgium herved

I rebased the MR on 11.x, I had to drop some commits and reverts that were causing conflicts so it'll be easier to rebase later.
We were previously using 🐛 Access denied to published private file if original translation is unpublished Needs review on our project but I can confirm this solution also covers it.
Also attaching static patch for composer.

🇧🇪Belgium herved

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

🇧🇪Belgium herved

Hi @mably, done.
It seems there might be an intermittent issue in UserIdsTest::testUserIdsByUserValues.
https://git.drupalcode.org/issue/purge_users-3542620/-/jobs/6289407

I was able to reproduce locally by running in a loop while ddev phpunit tests/src/Kernel/UserIdsTest.php; do :; done
But this is OOS.

🇧🇪Belgium herved

@gxleano Could you confirm you are open to the idea of dropping jquery in those behaviors?
If so I can try to find some time but I noticed and opened more pressing issues/bugs.

🇧🇪Belgium herved

I created a first draft but the tests could be improved IMO (assertions, methods,...).

🇧🇪Belgium herved

I encountered this issue as well, it looks like the tagify BEF widget expects the exposed filter to be of type "Autocomplete" and is incompatible with "Dropdown" at the moment.

🇧🇪Belgium herved

Hi @joelpittet, I know I combined 2 issues into 1 here, which may not be ideal, but they are tighly coupled.
In my understanding, considering this issue here, and the changes I propose, external files would and should never enter the ::optimizeGroup method. So does it really make sense to check for minified === TRUE && type === external in ::optimizeGroup as you proposed there? Meaning the condition is therefore not needed.

🇧🇪Belgium herved

Hello, if you are hitting "Only file JavaScript assets can be optimized" or "Only file CSS assets can be optimized" I just created 🐛 Assets paths in CSS no longer rewritten when aggregation is enabled Active which looks like a different case than the one reported here.

🇧🇪Belgium herved

Hello, thanks for creating this issue.
I did hit this as explained in #3414173-37: Add support for minified external CSS libraries (comments 37-39).
But I also hit another issue which relates to 🐛 Only file JavaScript assets with preprocessing enabled can be optimized. Active
Both are closely related so I opened 🐛 "Only file JavaScript/CSS assets can be optimized" errors in logs Active and proposed a slightly different approach.
Any feedback is welcome :) Thanks

🇧🇪Belgium herved

Tests and steps to reproduce still todo.
But any feedback is welcome.

🇧🇪Belgium herved

Opened MR
For context, I suspect we may have an infrastructure issue somewhere, possibly the reverse proxy serving stale pages.
Still, if that is the case, drupal should not fill the logs with errors. The changes here will log BadRequestHttpException warnings just like other invalid asset requests.

🇧🇪Belgium herved

Attaching static patch for composer.

🇧🇪Belgium herved

Hi, what is the purpose of this use_form details? just purely UI to put elements under this collapsible section?
If so I proposed a solution for this at [3452930-8] by using a #pre_render instead of #validate.

🇧🇪Belgium herved

I noticed a few issues and opened MR to address those https://git.drupalcode.org/project/spamspan/-/merge_requests/37

- At the moment the formatter settings with layout builder are not saved correctly and values under use_form are nested and not flattened. This happens because the form structure with layout builder is different than with field display overview form so the code in EmailSpamspanFormatter::validateSettingsForm is not correct.
- To solve this I removed this validate handler and use a #pre_render to display the use_form details while keeping the values flat.
This way it works on both and we do not need to know the entire form structure or make conditions.
- Added a test for the spamspan formatter and config schema tests for that and filter format
- Also centralized config schema under spamspan_plugin_settings so we can avoid repetition use it for the formatter and filter.

🇧🇪Belgium herved

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

🇧🇪Belgium herved

Updated IS, added MR and test coverage (the test fails correctly on D11 without the fix).
Adding also static patch for composer.

🇧🇪Belgium herved

Created MR, took the patch from #67 (2912092-67-10-1-x.patch), fixed eslint, and one twig missing commas.
Attaching static patch for composer.

Tests still needed. I had a look but we cannot simply replicate in \Drupal\FunctionalJavascriptTests\Core\Form\JavascriptStatesTest, this needs fields on an entity form for every scenario (e.g.: multi-value fields, date fields,...). Also media library fields if possible, which is why I need this patch.

🇧🇪Belgium herved

I rebased MR, fixed phpcs/phpstan and converted the hook to OO, tested on demo_umami and it looks fine.
Added 1 question in MR, this needs test coverage, so leaving to needs work.

Attaching also static patch for composer.

🇧🇪Belgium herved

phpcs fails, some more work needed

🇧🇪Belgium herved

I rebased the MR and added test assertions to address #47.
Attaching static patch for composer that applies on 10.2.x.

🇧🇪Belgium herved

MR !12521 seems to solve things partially.

Maybe the inline_form_error deserves its own issue, not sure...
But many errors are attached to the field itself (not properties) and in such cases inline_form_error links points to anchors that do not exist in the form. This happens on single element fields.

To me it's very strange and suspicious why field-multiple-value-form.html.twig does not print the outer div when not multiple, along with attributes and #id of the element. So something like
{% if multiple %}
...
{% else %}

{% for element in elements %}
{{ element }}
{% endfor %}

{% endif %}

This may be a decent way to solve that issue.

🇧🇪Belgium herved

@joelpittet yes I'm sure it's this commit and reading the code, it makes total sense.

IIUC the only thing \Drupal\Core\Asset\CssOptimizer::optimize does is to fix/rewrite URLs when aggregating, which is always welcome for CSS IMO. Maybe this is why it didn't match the same logic as JS?
IDK but I suspect this is going to cause issues for others as well when upgrading to D11.2, at least if they used minified: true and require url() in their CSS to be rewritten.

🇧🇪Belgium herved

This change seems a bit unexpected to me.
I didn’t read the full issue yet, but I have a theme that compiles and minifies SASS to CSS using Webpack, so I was using minified: true, assuming it would prevent additional minification by core.

After upgrading from D10 to D11.2, I noticed that url() references in my @font-face declarations are broken when CSS aggregation is enabled.
In 11.2, the CSS files retain relative url() paths, regardless of whether aggregation is enabled — which wasn’t the case before.
For now, I’ve removed minified: true from those files, even though they’re already minified by my build process.

Was this behavior change intentional?

🇧🇪Belgium herved

So far I see the code at core/modules/contextual/js/contextual.js:167 is attempting to get data-contextual-id elements that are not guaranteed to be there if these were relocated.

window.setTimeout(() => {
  initContextual(
    $context
      .find(`[data-contextual-id="${contextualID.id}"]:empty`)
      .eq(0),
    html,
  );
});

But this line hasn't changed in #3203920 so probably there is something more at play there.

🇧🇪Belgium herved

This patch can be combined with 🐛 PHP warnings from FileUrlWidget Needs review which fixes another error with the widget.
Maybe it's best to centralize both in a single issue?

🇧🇪Belgium herved

I opened a MR for the 2nd option: replace the URL autosubmit and JS with a "Add URL" button.
Because this is a much better approach for UX: an autosubmit on input change is very intrusive for the user and a button is a much better approach.

I did have to refactor the #access applied on elements because having it to FALSE causes havoc.
It seems in that case Drupal doesn't understand what button triggered the action in \Drupal\Core\Form\FormBuilder::handleInputElement and considers the first button in form_state was clicked. This causes all kinds of misbehaviors.

🇧🇪Belgium herved

- Installed latest core 11.x
- Enabled dependencies (jquery_ui, jquery_ui_draggable, jquery_ui_resizable) + bootstrap theme 3.x-dev
- Tested the navbar hamburger on mobile and collapse, all ok
- I can see both fixes from entreprise7pro/bootstrap are present
https://github.com/entreprise7pro/bootstrap/commit/00ee4669f6e24c3635a2b...
https://github.com/entreprise7pro/bootstrap/commit/51adf3b32846a78f3b8e7...
- +1 LGTM

🇧🇪Belgium herved

Noticed the same after updating from core 10.4 to 11.2.
This is a regression from 🐛 Prefix/Suffix not inline with autocomplete field Active and happens at least with multiple cardinality link fields.
Discovered and closed a duplicate issue Multi-Link Fields styling applying flex class Active .
Updated IS and title

The current MR in both issues seem to break/ignore what the parent issue meant to achieve.
Since I'm clueless in CSS, here is a patch that reverts the changes from #3471459.
Hopefully someone can provide a proper fix.

🇧🇪Belgium herved

This is a duplicate of 🐛 Link field description squashes the field Active which is older so I assume we can close this and centralize efforts there.

🇧🇪Belgium herved

Created new MR, the actual fix is in OpenDialogCommand::__construct
- 📌 Removal :tabbable usage in dialog.js Fixed was committed to 10.3.x and 11.x but their commit differs, mainly in OpenDialogCommand::__construct where the issue lies in 11.x, because if $dialog_options['dialogClass'] is set (it is via ckeditor5.js), then $dialog_options['classes']['ui-dialog'] is not taken into account.
- this MR contains improvements in order to ensure both $dialog_options['classes']['ui-dialog'] and $dialog_options['dialogClass'] are taken into account/preserved.
- 10.3.x is not affected by this bug per-se because it does preserve $dialog_options['classes']['ui-dialog'], however it could (should IMO) be ported as well, for consistency.
- I took the opportunity to align the way this is handled in OpenDialogCommand and OpenOffCanvasDialogCommand
- Added some regression tests for those + test to ensure the media-library-widget-modal class is present on the ckeditor media library modal.

🇧🇪Belgium herved

herved changed the visibility of the branch 3474018-alt to active.

🇧🇪Belgium herved

herved changed the visibility of the branch 3474018-alt to hidden.

🇧🇪Belgium herved

I can confirm the issue as I stumbled on this after upgrading to D11.
It's very easy to reproduce with demo_umami profile and opening the add media modal from ckeditor.
I just did a git bisect on the 11.x branch and
66c3914d39bf24d828ec8516389d7cadad52294e is the first bad commit and indeed comes from 📌 Removal :tabbable usage in dialog.js Fixed

🇧🇪Belgium herved

This ultimately needs maintainer decision but +1, looks good to me.
Without this patch we cannot have a tagify select field without this description.

🇧🇪Belgium herved

I tested manually both forms, adding a button that rebuilds the form, no errors anymore.
+1 LGTM, thanks

🇧🇪Belgium herved

I was able to reproduce easily, and the proposed changes are fixing the issue.

I just left a comment regarding the whole FieldConfigFormAlter/BundleEntityFormAlter/AbstractFormAlter/FormAlterInterface setup which is convoluted and unnecessary IMO, and could be done in just 1 service class, which may result in a more flexible architecture.
But this is probably OOS, and needs maintainer feedback.

Production build 0.71.5 2024