Fastest commit merge ever! 🙂 Thank you too
mxr576 → changed the visibility of the branch 3523983-fix-missing-config to active.
mxr576 → changed the visibility of the branch 3523983-fix-missing-config to hidden.
I am quite confident, you do not need DDEV to reproduce this issue at all, just clone the project and try to run composer install inside the folder. It is going to fail the same way because Composer cannot resolve other dependency versions without the branch alias.
A Slack conversation started here: https://drupal.slack.com/archives/C3E9QDZ5M/p1745499507960159
Where I have also shared this link that lists all contribs hosted on Drupal Git with DDEV enabled: https://git.drupalcode.org/search?group_id=2&scope=blobs&search=filename...
Yes, most of this is auto-genarated code at the moment of DDEV and necessary DDEV addons (if any is necessary) configured for the project. The benefit of committing these files that they drastically speed up contribution to projects, because whoever has DDEV installed - the official Drupal development tool for Drupal and contribs as far as I know - could just git clone the project and run ddev start and start contributing. They do not need to follow manual steps and ensure the right set of dependencies are installed, because the committed "last known good" configuration is in the version controlling system already.
Ignoring these configs from artifacts of course is a must, because when you install a module from an artifact you most likely do not need the bundled DDEV config.
Added test coverage, incorporated the latest fix from #13.
It looks like the best solution would be to have #2205027: VERSION in library declarations does not work for contributed/custom modules commited in core, so we can cache our assets as best as possible.
It could be, but until that is resolved, the current solution with version: VERSION leads to cache invalidation issues at client side and CDN level when only this module is updated.
So heavy +1 on removing this constant usage for now and let Drupal to calculate the version of the asset based on the hash of the file for now.
See also the warning on https://www.drupal.org/docs/develop/theming-drupal/adding-assets-css-js-... →
Checked quickly and I have only found a basic test coverage in the related sub module which might no be even using BEF in testing atm based on the export test view config :see-no-evil:
https://git.drupalcode.org/project/facets/-/tree/3.0.x/modules/facets_ex...
@smustgrave tried that, added a new checkbox multivalue widget to bef_view but could not reproduce the issue, not even with sorting enabled on items. So the real culprit is the way how facets module reorders the active item to the top.
Since Facets 3 switched to BEF as dependency, maybe it becomes inevitable having a compatibility test with it since more and more issues popping up in BEF's issue queue from there. This is how I also got here this week :-)
I do not know how to provide test coverage for this without adding Facets 3 as a dependency, did not find a way to configure a similar behavior as "The facets exposed filter is configured to order any selected options above deselected options." which is the culprit of the issue.
mxr576 → made their first commit to this issue’s fork.
Just opened the upstream issue since I was always interested about having official support for the unpack feature fundamentals in Composer: https://github.com/composer/composer/issues/12401
+1 on this, I have just wanted to open a similar ticket after I have seen New Recipe Unpack composer plugin → being published.
Opened ✨ Add search functionality to exposed options Active
This was available in Facets 2 since #3255454: Checkbox and Links searchbox widget extension →
+1 on this, but I guess this should be a BEF issue
mxr576 → created an issue. See original summary → .
Wow, I did not want to make these changes, so rolling back. Probably this is needs work due to the previous comment by @strykaizer, but first I would like to have a yey or nay on the proposed solution.
To keep this backwards compatible, we suggested introducing a setting to toggle the behavior.
For existing views, the default behavior should stay as is.
For new views, the default behavior can be "update the form with AJAX".
This also occurred to me as a potential BC layer if necessary, so I like the idea, but I am not going to have capacity to work on this right now.
I'd suggest pushing #3163299: Ajax exposed filters not working for multiple instances of the same Views block placed on one page first, w
May I ask why the order matters? Why this one cannot be fixed sooner than the other?
(I am pushing for eliminating the dependency based on the slow progress I have seen on both tickets from the past. However, if both gets active involvement then...)
Last night I have got an alternative idea that seems even more scoped the task at hand does not introduce any markup change. If this seems better than we probably need a change record to notify contrib/downstream developers that their exposed plugin implementation - unless it extends \Drupal\views\Plugin\views\exposed_form\ExposedFormPluginBase must set a new data attribute on the form.
Tried to actualize and improve IS.
using a data attribute as selector for AJAX (instead of an ID)
Maybe it worth mentioning that in the latest proposed fix for ✨ Exposed forms in a block are not currently updated when Ajax filtering is executed Needs work I have introduced a wrapper element around the form that might be also a valid alternative here, although as I wrote in #3032353-63: Exposed forms in a block are not updated by AJAX → I do not want to the two issue to become a blocker of each others.
mxr576 → changed the visibility of the branch 3032353-10.4.x-fix-only-backport to hidden.
Pushed a new fix to ✨ Exposed forms in a block are not currently updated when Ajax filtering is executed Needs work , please check and provide a review if you are also impacted by this core bug.
It worth mentioning that I had an interesting adventure/sidetrack where I tried to figure out how to render just the form in a way that it is not exposed inside the block to avoid attributes bubble up from the form to the block, it turned out to be a dead end.
https://drupal.slack.com/archives/C079NQPQUEN/p1746456106393489
I have spent hours understanding the root cause of the issue, examining proposed solutions, and analyzing the reasoning behind them. Below I'll respond to previous comments and explain my thought process behind the fix currently pushed to
MR#12048.
The AJAX controller must either replace the exposed form in the block or the block itself. I couldn't find a reliable way to identify and target the parent block(s) containing an exposed form - this also seemed beyond the scope of a controller that rebuilds a form. Therefore, I decided to target and replace the form itself (following @khiminrm's suggestion in #53), and I believe I've resolved the issues he mentioned by removing attributes from the form before replacing it.
@lendude provided crucial information in #12 explaining why the simple approach below doesn't work with multiple blocks and would break when 🐛 Views hardcodes exposed filter block form ID's which breaks AJAX when the same form is shown multiple times on one page Needs review is fixed. However, I disagree with #15 that this issue should be blocked by the other one, and I've sought a future-proof solution that will work regardless of which fix lands in Drupal core first.
$exposed_form_block_render_array = $view->display_handler->viewExposedFormBlocks();
$exposed_form = $this->renderer->render($exposed_form_block_render_array);
$response->addCommand(new ReplaceCommand("[data-drupal-selector={$exposed_form_block_render_array['#id']}] form", $exposed_form));
Since
🐛
Views hardcodes exposed filter block form ID's which breaks AJAX when the same form is shown multiple times on one page
Needs review
will make form IDs dynamic, they cannot be used to target previously rendered forms in blocks for replacement, as new instances will have different IDs than existing ones. While additional CSS classes could be added to forms (like in
the proposed fix for the other issue), finding the right class in an array of classes within the controller seemed fragile. This is why I decided to add a wrapper element around the form and store its ID as a data attribute, which can be easily retrieved and used in the controller.
The solution has been tested with Facets 3.x and Better Exposed Filters, not just with Drupal core's built-in exposed form plugin.
Additional reactions to previous conversations and proposed solutions:
#2 render context wrapping
The Controller always runs within an existing render context - I've verified this multiple times and couldn't find a way to run it outside a render context. Therefore, I don't believe the following workaround is necessary:
+ $context = new RenderContext(); + $exposed_form = $this->renderer->executeInRenderContext($context, function () use ($view) { + return $view->display_handler->viewExposedFormBlocks(); + }); + if (!$context->isEmpty()) { + $bubbleable_metadata = $context->pop(); + BubbleableMetadata::createFromRenderArray($exposed_form) + ->merge($bubbleable_metadata) + ->applyTo($exposed_form); + }
Or the workaround from
\Drupal\views\Plugin\views\style\StylePluginBase::renderFields():
// Views may be rendered both inside and outside a render context: // - HTML views are rendered inside a render context: then we want to // use ::render(), so that attachments and cacheability are bubbled. // - non-HTML views are rendered outside a render context: then we // want to use ::renderInIsolation(), so that no bubbling happens if ($renderer->hasRenderContext()) { $renderer->render($data); } else { $renderer->renderInIsolation($data); }
Due to the existing render context,
->render() bubbles up cacheability metadata out of the box.
Please correct me if I've missed something.
#17 "Add exposed_form_display option to the request." or not
#17 by @allaprishchepa should no longer be relevant since the solution targets the form rather than the block. The block inherits attributes (like the targeted ID selector in #17:
"#views-exposed-form-" . $view_id) when exposed in a block, which may have been the root cause of the problem.
Let's start with a clean sheet, I will provide in depth analyzes and details that hopefully justifies my decision.
mxr576 → changed the visibility of the branch 3032353 to hidden.
Read this thread and 🐛 Views - update filters block on ajax request Needs work . Also evaluated current and previous approached in patches and MRs.
Because this issue has found a close-enough solution to cover this issue with tests and there were more activity on that issue, I have closed the other one as duplicate and continue the work here.
Credits should be transferred from the other issue.
Read this thread and ✨ Exposed forms in a block are not currently updated when Ajax filtering is executed Needs work . Also evaluated current and previous approached in patches and MRs.
Because the other issue has found a close-enough solution to cover this issue with tests and there were more activity on that issue, please allow me to close this one as duplicate and continue the work in the other other.
Credits should be transferred to the other issue.
mxr576 → changed the visibility of the branch 2894747-10.1.x to hidden.
mxr576 → changed the visibility of the branch 10.1.x to hidden.
Thanks for the feedback!
Yes, I am also aware of the complexity of changing interface contracts; however as you mentioned, it is also a well known and standard procedure even outside of Drupal. Moreover, there is a recent interface change in Drupal core that I am aware of: https://www.drupal.org/node/3459274 →
PHPStan spots interface changes even on level 1 and PHPStan is native feature on Drupal CI and I am hopeful that nobody disables it ;S
Okay, I have certainly overlooked something, sorry about the noise - I was very surprising being the first one spotting this, but now I know why.
I tested what happens when an index’s server assignment changes, and wanted to clarify the resulting config values:
- If an index is explicitly set to “- No server -” in the UI, the config is saved as server: ''.
- If the server that an index is assigned to is deleted, the index entity ends up with server: null.
--- If you then edit and re-save such an index (now with no server) and again select “- No server -”, the value becomes server: ''.
I've set up a test environment to verify the behavior before and after applying the patch. The environment consists of:
- A test user (user1) who is the author of 2 published and 2 unpublished nodes (articles and pages)
- Important: user1 did not create these nodes - they were created by uid1 who set user1 as the author
- user1 has no special permissions beyond standard authenticated user permissions
- A simple Views view that lists content from a search index with only the Content Access processor enabled (similar to search_api_db_defaults setup)
- The search_api_test module and its replacement introduced in this MR (search_api_test_access) were enabled with the state flag that enables the node access feature
Before the fix
As shown in the screenshots, before applying the patch, user1 could not see the unpublished nodes they authored in the search results. Interestingly, they could see comments attached to these unpublished nodes - creating an inconsistent user experience.
After the fix
After applying the patch, user1 can now see the unpublished nodes they authored in the search results, which aligns with Drupal core's node access handling.
Made a first stab on this, tried to use atomic and detailed commits because some initial refactoring was necessary.
Opened a follow up and submitting a fix: 🐛 Fix spinning up local dev envs with ddev-drupal-contrib Active
mxr576 → created an issue. See original summary → .
Good thinking! I can confirm that the changes you made still mitigates the reported issue. Should I just RTBC this then? :thinking-face:
Please also note that my proposed fix also already occurs in the postSave() method, so my theory was that it was just forgotten to be added to this specific place as a guarding condition.
elseif (!$this->status() && $original->status()) {
if ($this->hasValidTracker()) {
$index_task_manager->stopTracking($original);
}
if ($original->isServerEnabled()) {
$original->getServerInstance()->removeIndex($original);
}
}
Shouldn’t the server be set to null instead of ''? The former means “no server”, the latter looks to most code like a server with ID ''.
Good question, I was also thinking about that, but the configuration that I have tried to import via Recipe was created by selecting " - No server - " in a search index and that lead to server: '' in the config instead of server: null.
There is a simpler way to handle potential null
Maybe the excerpt generator could also respect the "highlight returned field data" settings and just skip fields that are not returned by the server...
mxr576 → created an issue.
(Original Slack thread with the problem statement: https://drupal.slack.com/archives/C079NQPQUEN/p1744729430123259)
Yeah, well, that is part of the original description which I haven't changed, I have only fixed some typos and links.
That said, it is also true, since unless something changed in contrib since the issue was opened, the only code that calls the function and uses the $field parameter is in the test that proves that issue exists.
have search in contrib and was unable to find any attempt to use a non NULL value for this param.
Functional JS tests fail for no valid reason on the CI, so that can be ignored.
I think the proposed fix for 🐛 file_get_file_references does not return correct results when using $field param Active in https://git.drupalcode.org/project/drupal/-/merge_requests/11831 could also address this problem, please verify if you have time.
I have also bumped into this issue recently and after some forensic analyses my gut says that the $field based filtering was never worked as expected in 8.x.
Back to the history:
* https://github.com/drupal/core/commit/8aa449c8248e87e3f675503be1da462c8c... - nothing to see here, just use DI
* https://github.com/drupal/core/commit/de8bb2efaa607caf1d913288ca160f64c4... - Maybe this is where it all got broken, or even before...?
And we also have this CR related to the last commit: https://www.drupal.org/node/2260037 →
I have proposed a fix for it, I am uncertain at this point if this should be dropped or not, but should be fixed before # 📌 [PP-1] Deprecate file_get_file_references(). Move the logic to file.usage service Postponed if it stays.
Tried the latest patch from this module, still applies on Drupal core 10.4.x. I did that after I also checked
https://www.drupal.org/project/file_replace →
- for my use case, both of them have the same limitation which may worth considering in the scope of the final solution: How to access to the replaced file content in hook_entity_update()? Because currently it is not possible, $file->original is pointing to the same file entity when hook_entity_update() is called and the content of the file is already replaced when that hook is called - so if you would like to run some business logic based on file content changes, you cannot.