I was recently looking for a way to append an item to a list property on a config entity. After some discussion on Slack and a bit of Googling, I found this issue and the proposed solution.
A few important questions came up that should be discussed and decided:
- Should the scope of this issue/MR be expanded to include list property manipulation on config entities? The current state of the MR explicitly excludes support for config entities.
- If config entities are included, how should the action be named? The current name, "simple config array", would no longer be accurate. Should we introduce a separate set of actions for simple config and config entities, or find a naming approach that can cover both?
For reference and potentially for consistency it might be worth mentioning that support for config entity updates was recently deprecated in simpleConfigUpdate: https://www.drupal.org/node/3515543 → .
And the answer was in front of my eyes after my new RCA... Gotta go to the corner to cry.
Thanks for saving me from another hours of debugging and sorry for the noise.
mxr576 → changed the visibility of the branch 3526781-set-config-synching to hidden.
Okay, at this point, I am unsure where to catch this bug and which layers are impacted... of course setting is synching on the config installer is not a god idea since it triggers #3 by design (from \Drupal\Core\Config\ConfigInstaller::createConfiguration():
// If we are syncing do not create configuration entities. Pluggable
// configuration entities can have dependencies on modules that are
// not yet enabled. This approach means that any code that expects
// default configuration entities to exist will be unstable after the
// module has been enabled and before the config entity has been
// imported.
if ($this->isSyncing()) {
continue;
}
setting the isSynching() on the config entity was an idea to bypass the dependency calculation in \Drupal\Core\Config\Entity\ConfigEntityBase::preSave():
if (!$this->isSyncing()) {
// Ensure the correct dependencies are present. If the configuration is
// being written during a configuration synchronization then there is no
// need to recalculate the dependencies.
$this->calculateDependencies();
// If the data is trusted we need to ensure that the dependencies are
// sorted as per their schema. If the save is not trusted then the
// configuration will be sorted by StorableConfigBase.
if ($this->trustedData) {}
...but this changes Drupal core behavior globally, there is a way to scope it to recipes only, but still, would that be the approach? Unsure, especially even with this change other parts of the Recipe install fails when it tries to fetch the "datasource_foo" entity for different reasons (like because entity insert hooks are triggered).
New Drupal focused contents are on the horizon - one is in the review phase already - this is the reason why we thought it is also high time to get back on the Drupal Planet feed and not just promoting Drupal and our contribution on social media and other platforms.
We could also audit our old content and re-tag them with our new tag to appear in the feed, but that would not matter since the aggregator ignores older content.
and the exception originates from \Drupal\Core\Recipe\RecipeRunner::installContent()... so maybe I bumped into another issue and the proposed fix is not completely a bad idea (open for comments, please!)
Okay, so it seems the proposed change breaks the RecipeQuickStartTest with a similar error that I have reported, although the proposed fix fixes the problem that I reported.
In DiscoveryTrait.php line 53:\n
\n
The "entity:shortcut:default" plugin does not exist. Valid plugin IDs for Drupal\Core\TypedData\TypedDataManager are: filter_format, entity_reference, entity, entity:block, entity:block_co \n
ntent_type, entity:block_content, entity:field_config, entity:field_storage_config, entity:filter_format, entity:path_alias, entity:shortcut_set, entity:shortcut, entity:menu, entity:actio \n
n, entity:user_role, entity:user, entity:view, entity:date_format, entity:entity_view_mode, entity:entity_view_display, entity:entity_form_mode, entity:entity_form_display, entity:base_fie \n
ld_override, field_item:link, field_item:text, field_item:text_with_summary, field_item:text_long, field_item:float, field_item:integer, field_item:decimal, field_item:string_long, field_i \n
tem:created, field_item:entity_reference, field_item:string, field_item:changed, field_item:map, field_item:password, field_item:timestamp, field_item:uuid, field_item:language, field_item \n
:uri, field_item:email, field_item:boolean, float, decimal, boolean, email, uri, list, language, map, string, duration_iso8601, timespan, timestamp, any, binary, datetime_iso8601, integer, \n
language_reference
For the record, having server: null in the config also triggers this bug. So other than the inconsistency between '' and null as potential values, I think the fix in the MR is still necessary.
RTBC++, an essential addition!
mxr576 → changed the visibility of the branch 3163299-views-multiple-ajax-exposed-filters to hidden.
mxr576 → changed the visibility of the branch 9.2.x to hidden.
mxr576 → changed the visibility of the branch 3163299-ajax-exposed-filters-9.5.x to hidden.
mxr576 → changed the visibility of the branch 3163299-ajax-exposed-filters to hidden.
mxr576 → created an issue. See original summary → .
The follow up ✨ Improve error reporting when Composer state missmatch Active .
I ran into the same issue yesterday with a Recipe that depends on BEF. BEF includes a composer.libraries.json file in its package, which you can see here:
https://git.drupalcode.org/project/better_exposed_filters/-/blob/7.0.x/c...
I agree that Wikimedia’s setup is quite complex. In our monorepo, we also treat it as a necessary evil rather than something to optimize around, so I don’t think compatibility with it should be a primary concern.
To avoid this issue in our setup, we updated our include rule from the broad composer.*.json to something more specific like composer.dev.json, which prevents BEF’s composer.libraries.json from being picked up unintentionally.
That said, I’d recommend opening a follow-up issue to improve the error reporting in the assertion logic, so it provides clearer feedback when Composer’s actual state doesn’t match the expected configuration.
The current MR diff cannot be installed with Composer patches due to changes on composer.json - that not considered anyway by Composer .
@claudiu.cristea Could you provide more context on this issue? I wonder if 🐛 View recalculated with wrong data from cache Needs review will fix this also.
@drunken monkey I would suggested merging this MR and then other Views caching related issues - if they still exits - could branch of from a new baseline.
Bubbling up cacheabilty information from the display handler may not be enough, the cacheability information from the View entity should be also incorporated - at least cache tags, that includes the View config entity's own cache tag - more details in #2887144-44: Views exposed form block options are not updated immediately when adding additional sorts, filters, etc (Caching?) →
Let me also deeplink the failing test which should prove a cache invalidation issue still exists: https://git.drupalcode.org/issue/drupal-2887144/-/pipelines/500971/test_...)
Could be an upstream issue?
The failing test confirms that even with 🐛 Extend ViewsBlockBase to merge cache metadata from display handler Active merged into Drupal core, this issue still persists. The cacheability metadata from the View config entity is still not bubbling up to the block, so the underlying problem remains unresolved.
Relying on View::getCacheTags() should be a better solution instead of a hand-crafted config cache id, since \Drupal\views\Entity\View::addCacheMetadata() ensures that before a View config is changed display plugin's cacheability information is also incorporated into the stored cache tags.
Question: What about cache contexts and cache max age bubble up from Views?
The merge request is currently failing due to the test coverage introduced in
🐛
Extend ViewsBlockBase to merge cache metadata from display handler
Active
, which added significant amounts of mocking. Somebody has to fix those, because I tried and failed miserably due to interconnected dependencies and protected properties... (This is one of the main reasons I try to avoid overusing mocks in my own projects - they tend to make the test suite fragile and harder to maintain. </personal_opinion>)
(There may also be a related issue in the contrib space: 🐛 Filter configuration changes does not propagate immeditelly to BEF in a block Active .)
I would consider this an outdated issue, I have tried to reproduce the issue by following the manual steps on 11.x today and I could not.
My gut says that this issue was resolved in 🐛 Extend ViewsBlockBase to merge cache metadata from display handler Active since the merge was merged fix is similar to #13.
Please re-open if you disagree.
let's start with a clean up
mxr576 → made their first commit to this issue’s fork.
Meh, duplicate of 🐛 TypeError: Drupal\Component\Utility\Html::escape(): Argument #1 ($text) must be of type string, null given Active which is resolved... sorry for the noise.
Tested on my local and the fix works. Also ran the update hook.
I left a nitpick, but the current solution also works. RTBC++
I would first merge the soft limit fix and meanwhile flash out the details of this one, since this have to be configurable as well - even if it is 3 lines of CSS - it is more work the deliver.
Currently I also cannot commit to any dedicated time working on this.
. But eventually probably should have a facets related test added
Agreed, thank you!
@mably, that could also work, but I did not want to touch the core logic of filtering since fixing this specific problem was quite simple and did not require any dirty workarounds.
Also opened 🐛 Do not add leading/trailing ellipses when the string is less than excerpt lenght Active
This still seems the case.
I would suggest creating a dedicated issue for that and please rtbc this one if it worked for you.
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