🇧🇪Belgium @herved

CI failure looks related to https://git.drupalcode.org/project/htmx/-/commit/71890ab8a300e408ba7a7fd...

herved → created an issue.

herved → created an issue.

herved → created an issue.

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

Static patch of MR 10421

herved → changed the visibility of the branch 3416735-register-streamwrappers to hidden.

Found another possible regression, see #3416735-10: Stream wrappers not registered when installing module's default config →
Which I suspect is coming from ::updateKernel call at the very end of ModuleInstaller::doInstall

I stumbled on this issue because I found a scenario where stream wrappers are not registered properly.

When more than 1 module is enabled, stream wrappers are not registered, see https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal/Co... which calls $this->updateKernel([]) and doesn't call \Drupal::service('stream_wrapper_manager')->register(); after it.
Notice that just above it we have a call to ::updateKernel and then it registers stream wrappers.
That particular code seems to originate from 📌 Add the ability to install multiple modules and only do a single container rebuild to ModuleInstaller Active .

This can cause all kind of issues.
In my case, I have a config import that has to enable more than 1 module and delete an image style, and noticed the image style folder is not being deleted, which should happen in \Drupal\image\Entity\ImageStyle::postDelete but stream wrappers are not registered.

LGTM +1

I can confirm the issue and was able to replicate. The changes LGTM and address the issue. +1

Makes sense to me, an error is too "aggressive", so +1, thanks.
Here is a static patch for composer

Current MR snapshot for composer

This issue primarily focuses on ensuring that the locked and hidden states of facet processors are respected. Currently, facets_exposed_filters do not honor these settings. Some projects, including mine, rely on custom processors that depend on those features.
I believe the MR addresses the issue, but we could definitely add some test coverage.

I’d appreciate maintainer feedback on whether this is headed in the right direction.
While working on it, I realized the way FacetsFilter deals with the hierarchy processor and settings needed some adjustments.
I wonder why is the hierarchy processor locked in the first place, and if we could/should make it standard/unlocked. But that would involve a lot of changes also for facets blocks.

Just a thought, but the issue title is no longer accurate since the PHP TypeError doesn't happen anymore on 3.0.x.

So this now adds return type to SearchApiDisplay::getCount which could be done but this could affect BC and I wonder if we should apply the same to other ::getCount implementations for consistency? as mentioned in #13

I assume the reasoning behind this makes sense, but I noticed it can have side effects.
The added checkbox class breaks the styling on one of our projects that uses Bootstrap 3 and didn’t account for it.

I wonder if this should instead use the Drupal.theme JS API.
Isn’t form-check Bootstrap-specific?
Also, does this follow classes from core/modules/system/templates/form-element.html.twig? if so, why isn’t form-item included?

There is a conflict in the MR, recent changes on 3.x affected this and the return type was removed to fix phpstan.
See https://git.drupalcode.org/project/facets/-/commit/28332851b927d94f4deb3...

I think we could either:
- close this
- or add the return type to ::getCount in FacetSourcePluginInterface, FacetSourcePluginBase and SearchApiDisplay, but that would affect backwards compatibility I assume.

herved → created an issue.

Answering to #24:
1. If I'm not mistaken, this looks like a crude attempt to check entity access.
AccessManager::checkNamedRoute I used here is a much cleaner alternative.
2 & 3. The access cacheability is collected with the current MR here in the formatter, but we indeed still need a test here to validate the access is respected and the cacheability as well if possible.
I'll work on that next.

For now, I used a different solution for my project.
But I just wanted to open this to start the discussion.

herved → created an issue.

I agree with #28, displaying inaccessible URLs is a totally valid use case.
So indeed this is not a bug, and a formatter settings does make sense.
When I created this issue, I was confused why \Drupal\Core\Url::toRenderArray had the access check, but \Drupal\Core\Url::toString didn't. But I see now that 📌 [11.x] Remove deprecated code from \Drupal\Core\Url Active removed ::toRenderArray.

I also agree with all review comments so if anyone wants to address those, please feel free. Or I will find some time later this week.
Cheers.

It seems the main goal of ComponentSlotForm::elementValidate is to remove some values (add_more_button, _remove) from the final submitted form values which ultimately ends up in config.
I think it should not alter form elements ['#value'] but instead use $form_state->unsetValue() in elementValidate. There are some examples in core where this is pattern is used.
So I updated accordingly.

Moving back to needs work, the LB configure form closes unexpectedly with this when adding something in a slot.
I have to dig further, possibly split off the 2nd issue.

Could you please review this? It seems there are 2 issues:
1. states api not working
2. and I made an attempt for the ComponentForm::elementValidate and related code

Unfortunately there is no test coverage for this ComponentForm::elementValidate
So I'm not sure what to make of it...
Looking at the gitlog, it originates from:
- https://git.drupalcode.org/project/ui_patterns/-/commit/509f99793542456e...
- https://git.drupalcode.org/project/ui_patterns/-/commit/b2216dcb5daa8653...

Moving to review to get some feedback.

Created MR, this solves states not working, but the "enabled" key is not saved when using the TimestampFormatter, causing PHP notices.

Warning: Undefined array key "enabled" in Drupal\Core\Field\Plugin\Field\FieldFormatter\TimestampFormatter->viewElements() (line 319 of core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampFormatter.php).

Possibly something to do with the fact it uses #tree ?
$form['time_diff']['#tree'] = TRUE;

herved → created an issue.

Thanks @damienmckenna

Note that, for completeness, I discovered this because it can break layout builder which generates entities for the preview (in \Drupal\layout_builder\Plugin\SectionStorage\DefaultsSectionStorage::getContextsDuringPreview) if the formatter assumes it has valid json (I am using a custom formatter).

I could use some help, but this language subscriber is very suspicious...
For now one workaround is to uninstall modules with a post_update hook instead of letting config:import do it.

Adding is_callable causes 1 test fail: ModuleHandlerTest::testImplementsHookModuleEnabled

Loading module files before updateKernel causes 1 test to fail: ClassLoaderTest::testAutoloadFromModuleFile
Here is the stacktrace:

Error: Class "Drupal\module_autoload_test\SomeClass" not found in include_once() (line 12 of core/modules/system/tests/modules/module_autoload_test/module_autoload_test.module).
Drupal\Core\Extension\Extension->load() (Line: 143)
Drupal\Core\Extension\ModuleHandler->load() (Line: 323)
Drupal\Core\Extension\ModuleInstaller->doInstall() (Line: 229)
Drupal\Core\Extension\ModuleInstaller->install() (Line: 83)
Drupal\Core\ProxyClass\Extension\ModuleInstaller->install() (Line: 503)
Drupal\system\Form\ModulesListForm->submitForm()
call_user_func_array() (Line: 105)
Drupal\Core\Form\FormSubmitter->executeSubmitHandlers() (Line: 43)
Drupal\Core\Form\FormSubmitter->doSubmitForm() (Line: 616)
Drupal\Core\Form\FormBuilder->processForm() (Line: 348)
Drupal\Core\Form\FormBuilder->buildForm() (Line: 73)
Drupal\Core\Controller\FormController->getContentResult()
call_user_func_array() (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 633)
Drupal\Core\Render\Renderer::Drupal\Core\Render\{closure}()
Fiber->start() (Line: 634)
Drupal\Core\Render\Renderer->executeInRenderContext() (Line: 121)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext() (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 183)
Symfony\Component\HttpKernel\HttpKernel->handleRaw() (Line: 76)
Symfony\Component\HttpKernel\HttpKernel->handle() (Line: 36)
Drupal\Core\Test\StackMiddleware\TestWaitTerminateMiddleware->handle() (Line: 53)
Drupal\Core\StackMiddleware\Session->handle() (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle() (Line: 28)
Drupal\Core\StackMiddleware\ContentLength->handle() (Line: 118)
Drupal\page_cache\StackMiddleware\PageCache->pass() (Line: 92)
Drupal\page_cache\StackMiddleware\PageCache->handle() (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle() (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle() (Line: 53)
Drupal\Core\StackMiddleware\AjaxPageState->handle() (Line: 54)
Drupal\Core\StackMiddleware\StackedHttpKernel->handle() (Line: 716)
Drupal\Core\DrupalKernel->handle() (Line: 19)

it's also relevant that you have admin language negotiation enabled

Exactly, this is why I used demo_umami to reproduce the issue I have on my project.

I identified https://git.drupalcode.org/project/drupal/-/commit/78813e01c72ff50265df2... as the first failing commit which was introduced in 11.2.3. Issue: 💬 Drupal 11.2 upgrade causes \Drupal::$container is not initialized yet error Active
So this could be seen as regression.

#4, indeed that also seems to work.
OTOH it seems a bit strange to me that devel_entity_type_alter gets called while devel is not installed yet. Is that a concern?
At this stage, it is still in the preparation phase calling $this->updateKernel in ModuleInstaller::doInstall.

I'm not sure what is the safest option to apply until a proper fix is found. I may go with the is_callable option for now...

Here is the xhprof export of 2.0.9 + MR diff
With xhprof, the request takes 9.6s and SourcePluginManager::getDefinitionsForPropType and below takes 86%.

- 2.0.9: 13.39s
- 2.0.9 + MR diff: 2.55s
- 2.0.6: 2.23s

This is definitely better, thanks.
OTOH this is on a fairly powerful machine where standard LB configure block form request takes only a ~80-100ms to load in comparison.
So the context handling of ui_patterns is still extremely expensive.
This is from Drupal\ui_patterns\SourcePluginManager::getDefinitionsForPropType and below which takes ~90% of the total request time and the call counts below it seem a bit crazy.
It does solve the immediate issue but I still believe that something should be done to further improve this. I did try to investigate but this is definitely not an easy implementation.

Thank you @Bedir, indeed it looks like a different issue but possibly with the same root cause?
I created 🐛 Uninstalling and installing modules during config:import can lead to fatal errors Active . Would it make sense to add a check to is_callable($callable) in ModuleHandler::getFlatHookListeners ?

Observations (so far):
- In that example, it seems to involve \Drupal\language\EventSubscriber\LanguageRequestSubscriber::onContainerInitializeSubrequestFinished, which gets called when devel gets installed. The container gets -reinitialized and triggers all the language negociation handlers and eventually hook_entity_type_alter.

This doesn't seem to be a problem when the deployment only contains a module install.
However when a module gets uninstalled (i.e.: page_cache), then ModuleInstaller::uninstall clears cached (entity) definitions at the end (see "// Flush all persistent caches.")

- Applying the patch from 🐛 ModuleHandler skips all hook implementations when invoked before the module files have been loaded Needs review solves it, but does it really make sense that devel_entity_type_alter gets called while devel is not actually installed yet?
- Another possible fix might be to ensure the hook is actually callable before adding to the list in \Drupal\Core\Extension\ModuleHandler::getFlatHookListeners

--- a/core/lib/Drupal/Core/Extension/ModuleHandler.php
+++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
@@ -752,7 +752,7 @@ protected function getFlatHookListeners(string $hook): array {
           else {
             $callable = $listener;
           }
-          if (isset($this->moduleList[$module])) {
+          if (isset($this->moduleList[$module]) && is_callable($callable)) {
             $this->listenersByHook[$hook][] = $callable;
             $this->modulesByHook[$hook][] = $module;
           }

Would that be a better solution?

herved → created an issue.

Yes, rolling back to 2.0.6 significantly improves performance compared to 2.0.9.
The same LB configure form goes from around 13s on 2.0.9 to 2-3s on 2.0.6.

I believe I am facing this bug on my project.
This happens during deployment in the config:import phase, when there are both modules to be uninstalled and installed at the same time.
I did manage to reproduce on clean 11.x with demo_umami and devel

Steps (I use ddev):
- ddev drush si demo_umami -y
- ddev drush cex -y
- Now go to sites/default/files/sync/core.extension.yml:40 and replace page_cache: 0 with devel: 0
- ddev composer require --dev drupal/devel
- ddev drush deploy -y

 Error: Call to undefined function \devel_entity_type_alter() in /var/www/html/core/lib/Drupal/Core/Extension/ModuleHandler.php on line 479 #0 /var/www/html/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php(389): Drupal\Core\  
  Extension\ModuleHandler->alter()

Stacktrace: https://gist.github.com/vever001/a80414b43d317d84e1d1aacc91cbb895

It seems to involve \Drupal\language\EventSubscriber\LanguageRequestSubscriber::onContainerInitializeSubrequestFinished.
Maybe there should be a check to \Drupal::isConfigSyncing() somewhere?
But why is it attempting to run devel_entity_type_alter while devel is not even installed yet.

Hi @just_like_good_vibes, I'll have a look shortly and report back today, thanks.

No problem, for now I decided to not rely on ui_styles_entity_status and opted for a custom hook_entity_view as I have other features applying background styles.

- uasort
-- \Drupal\ui_patterns\SourcePluginBase::label
--- $this->getChoices()
Seems to trigger a lot of unnecessary trips to context handling and \Drupal\Core\Plugin\Context\ContextHandler::filterPluginDefinitionsByContexts
Eventually it doesn't use any of those and fallbacks to last line in \Drupal\ui_patterns\SourcePluginBase::label
Stacktrace: https://gist.github.com/vever001/e5f5721be404e9d4ce931d6df3dc208f

herved → created an issue.

herved → created an issue.

herved → created an issue.

+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.

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.

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!

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".

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.

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.

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.

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.

MR rebased, attaching diff for composer

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

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

MR rebased

MR rebased

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();
}

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.

Actually, a few more are missing, see #3544226-5: Misplaced schema definitions (wrong submodule) →

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?

#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

herved → created an issue.

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

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

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.

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