@godotislate yes that is possible. Symfony process allows commands to be sent async,which can be done within a fiber, and then while waiting/polling that to come back, Drupal can do other things like send database queries.
Moving this back to 11.x. However hook_module_implements_alter() won't be in 12.x at all (although most other procedural hooks will be eventually deprecated for removal in 13.x, not 12.x).
Bot is understandably confused by the multiple branches. Closed all MRs except the active one.
I've unpublished the two CRs (so it's reversible), which leaves the redirects. I don't think I have permissions on d.o to set those up.
The test changes will need to use phpunit attributes instead of @group legacy now.
With regards to #16 is it possible we could run into a scenario though where the CI downloads a newer version that breaks something
Yes we've fairly regularly had baseline changes due to phpstan updates (items removed or added due to bug fixes usually), and also we need people's local installs of phpstan to be the same as what's on CI.
Committed/pushed to 11.x, thanks!
Nice to remove all that cruft.
Committed/pushed to 11.x, thanks!
Back to needs review. https://git.drupalcode.org/project/drupal/-/merge_requests/13252 is the one to review now.
Could we not add the cache.static deprecation to the ignore list?
We should open a spin-off about that maybe - could probably check if the library is deprecated or not and allow empty deprecated libraries, but it shouldn't block this one.
Tried to search for usages and didn't get useful results, but it did find one components/content_edit/node_edit_form/node_edit_form.component.yml
https://git.drupalcode.org/search?group_id=2&scope=blobs&search=node%2Ff...
We'd normally add it as a JavaScript development dependency (same as cspell, eslint etc.) and then it will be installed in the general yarn install that CI pipelines already do. That way it's also available for people running the tests locally.
@godotislate
One other option, though I'm not sure it's better: in escapeFilter() , return a clone of the object if an Attribute object is passed in.
I was doing this in an earlier revision, but didn't really want to special-case Attribute because something else could theoretically be implemented similarly and suffer from the same problem.
I wonder why we didn't do this in the first place.
I thought about this, and it looks to me like we just wanted to pass through anything that doesn't get sanitized with the least amount of change + it will get cast to a string almost immediately after this logic runs anyway. The method name is only about sanitization not about casting, so there might have been a reluctance to do more than was advertised too.
Between 5 and 7 that sounds fine to me - as long as we've got a way to run a test but have some kind of ground where we can assert the things that should pass, while adding todos for things that don't - that way it's not a choice between no tests and failing tests. We should really try to get 📌 Automated A11y tests in PHPUnit Active done though.
It would be possible to leave the library empty and deprecate it for removal in Drupal 12 maybe - that would notify when something attaches it or depends on it.
There was a merge commit in the branch history which made things impossible to rebase. I started a new branch, reverted to the last known good commit, then rebased from there - have hidden the other branches for now.
catch → changed the visibility of the branch 3496369-preload-without-cache to hidden.
Committed/pushed to 11.x and cherry-picked to 11.2.x, thanks!
Rebuilt the phpstan locally. Committed/pushed to 11.x, thanks!
OK let's get this one in - will mean rebasing the issues fixing the baseline but can't be helped.
Committed/pushed to 11.x, thanks!
Most of the calls are still from Cache::mergeContexts(), but this only happens when assertions are enabled (which they are on my local environment where I profiled). For me this was 1/3rd of the calls with cold caches, and much more than half the calls with a warm cache - so I think this is probably outdated.
Updated the issue summary. With this approach I don't think we need a change record for Twig template authors, but we probably need a general one in case someone somewhere is doing something weird with print in Twig nodes or something.
OK pushed another change that now early-casts for all MarkupInterface objects, this would mean anything attribute-like either now or in the future would also be covered.
Without use_yield, the MarkupInterface objects that are preserved in TwigExtension::escapeFilter() get immediately cast to a string anyway as soon as they leave it, with use_yield that can be slightly delayed which causes the problem here.
While this changes which method they get cast to a string by, I'm pretty sure it's only a fraction earlier with nothing else happening to them in-between, which allows us to keep the behaviour transparent for everything else.
If I'm overlooking something, we could go back to only casting Attributes early and handle the rest in a follow-up.
I looked at what was going on with Twig rendering a bit more.
Here's the relevant output from the actual generated twig template (e.g. what gets written to sites/default/files/php/twig):
// line 48
yield "
<ul ";
// line 49
yield $this->extensions['Drupal\Core\Template\TwigExtension']->escapeFilter($this->env, CoreExtension::getAttribute($this->env, $this->source, CoreExtension:: getAttribute($this->env, $this->source, ($context["attributes"] ?? null), "addClass", ["primary-nav__menu", ($context["primary_nav_level"] ?? null)], "method", false, false, true, 49), "setAttribute", ["data-drupal-selector", ($context["drupal_selector_primary_nav_level"] ?? null)], "method", false, false, true, 49), "html", null, true);
yield ">
";
// line 50
$context["attributes"] = CoreExtension::getAttribute($this->env, $this->source, ($context["attributes"] ?? null), "removeClass", [($context["primary_nav_level"] ?? null)], "method", false, false, true, 50);
// line 51
yield " ";
I was confused why the ::escapeFilter() call doesn't cause the attribute to cast to string there, because Attributes implement ::__toString()
However, escapeFilter() passes through MarkupInterface objects without casting to string. Now I actually understand what's going on finally (maybe).
I've pushed a change that reverts all the template changes, and special cases atrributes in escapeFilter so that they get cast earlier.
Olivero menu rendering still works, let's see if this causes any other test failures though. If it works and doesn't cause other problems, that lets us switch to Yield with no changes for template authors.
I feel like we could even go a step further and cast any MarkupInterface object to a string in ::escapeFilter(), leaving only Twig Markup object completely intact there. But will see if we get a green test run first.
That is indeed very annoying.
Had a play around with different approaches, if we make sure create_attribute also clones, we can skip the toArray() - same overall approach though.
Pushed a commit for the original MR for that.
The nightwatch failure is some kind of twig problem in the other issue, but confirmed that issue fixed the remaining thing here, so marking postponed on that.
This is (at least part of) the markup difference:
-<ul class="menu primary-nav__menu primary-nav__menu--level-1" data-drupal-selector="primary-nav-menu--level-1" data-once="olivero-automatic-mobile-nav">
+<ul class="menu primary-nav__menu" data-drupal-selector="primary-nav-menu--level-2" id="primary-menu-item-1">
@@ -17,7 +17,7 @@
<span data-drupal-selector="primary-nav-menu-🥕" class="primary-nav__menu-🥕"></span>
- <ul class="menu primary-nav__menu primary-nav__menu--level-2" data-drupal-selector="primary-nav-menu--level-2" id="primary-menu-item-1">
+ <ul class="menu primary-nav__menu" data-drupal-selector="primary-nav-menu--level-2" id="primary-menu-item-1">
The missing class is added in the olivero primary menu template with ::addClass(). This happens inside the menu_links macro so it's very possible it's something to do with the macro/recursion, no idea what though.
The nightwatch failures are reproducible with the standard profile - you need to add a second level of menu items, then things get weird.
It looks like it's possible to have twig nodes that aren't compatible with yield - e.g. that print or echo, but I can't see any sign of anything like that yet.
I think the signature should be more like ::assertStringOrder($items, $markup) - we can then get $markup from the page, but also from direct rendering or other sources. I needed something like this for 🐛 Twig output buffering is incompatible with async rendering Active and found this issue.
Making it clear we're changing a Twig option here and not rewriting how it renders templates.
Confirmation that this works: https://git.drupalcode.org/issue/drupal-1237636/-/jobs/6521003
Root cause of the Umami test failure is 🐛 Twig output buffering is incompatible with async rendering Active . I've incorporated the fix here to see if we can get to green.
Draft test only MR pipeline shows the correct test failure: https://git.drupalcode.org/issue/drupal-3546376/-/jobs/6520638#L4067
Added some test coverage.
(Note: a percentage of the test coverage was written using Claude cli, this is because Tag1 sponsored some time to work on any core/contrib issue, but specifically using Claude in order to evaluate it. I chose this one because I knew if I was able to track down the bug that I'd want to try to write a kernel test for it that didn't involve nested media/image rendering (which is what surfaced it in the entity loading issue). I don't think it saved any time overall since things constantly go wrong and it takes as much time thinking how to prompt it to get what you want as it would to have typed it out. It probably did save a few minutes making a stub twig function since that's something I never do normally, and suggested it when I asked it for some alternative ways to execute PHP code during Twig template rendering - probably would have go there myself but hadn't yet at the point I prompted. - so a mixed bag).
Thanks for the quick MR. Committed/pushed to 11.x, thanks!
MR may be the most hours per lines of code I've ever submitted.
The bug was initially surfaced via a single Umami test failure in 📌 Entity lazy multiple front loading Active - this asserts the order of various content on the Umami front page.
What was happening with the bug itself was elusive for a while. Initially I thought that placeholders and their output were getting 'swapped' somehow. Need to open a couple of spin-off issues for cleaning up code that ultimately turned out not to be a problem, will link from here once that's done.
Closer inspection of the lazy builder markup output indicated that instead of being swapped, bits of placeholders were getting mashed together in the output of different placeholders. This was even more confusing and I still couldn't track it down. We're not assigning output to a static cache or anything like that anywhere, Fiber rendering can't be doing anything that weird even to recursive rendering of render arrays etc.
Then in the shower this morning (really) I remembered that Twig uses output buffering. So once the output buffer was started, different bits of different lazy builder markup were going into the output buffer depending on which placeholders suspended fibers.
Just a note I briefly thought 'could we put this logic in a closure' - that would allow for the multi-line logic, but not for re-use, however I think there's enough here to break it out.
Had one question on the MR about the parameter change on EntityController - but also there are merge conflicts in the MR.
This makes sense to me, tagging needs follow-up for the idea of contextual links from the block going back to the layout builder edit page. But also I wonder what XB/Canvas is going to end up doing with things like that whether for embedded entities or SDC content.
Committed/pushed to 11.x, thanks!
@dww 📌 Add Drush command to allow running cron updates via console and by a separate user, for defense-in-depth Fixed allows you to configure a cron job to run automatic updates, this would (theoretically at least) allow someone to configure automatic updates to only ever do anything via that cron job with a non-functioning UI (because the UI doesn't have permission to write to the file system). But yeah the capability to have the UI work with non-web-writable directories was never added and is what this issue is open for.
The UIs themselves aren't in core, but to me it makes sense to have the queue implementation in package_manager so that both automatic updates and project browser can be updated to work with it.
when it's meant to be a drop-replacement for a persistent cache, like we do in kernel tests for example.
In kernel tests we're using the test-only memory cache we added before any of this that does serialize()/unserialize(), this is because otherwise the behaviour of setting/getting an item from the cache is different to if it's in the database or apcu etc. which would not be good for kernel tests.
For the actual memory cache we don't want to serialize/unserialize, but this means it isn't a 1-1 drop in replacement for persistent caches - obviously you can swap them around if you realise something does/doesn't need persistent caching but it does behave differently, which is why we added MemoryCacheInterface.
📌 [PP-1] Clean up MemoryCache and MemoryBackend naming issues Postponed is open to clean all of that up.
cache.backend.memory should not be using MemoryBackend, that's only reference by cache.backend.memory - we could probably clean that up here - make cache.backend.memory.memory an alias and deprecate it, so we're not using the testing code in runtime.
Discussed this briefly with @lauriii, he asked why we're adding a feature flag for this at all - it only affects existing sites when they add a field in the field UI, not the actual runtime of the site - so we should be fine to just change the behaviour with no bc layer.
Thinking about it more this makes sense to me too, I tried to trace back where this was discussed and I see Damien's comment asking whether it was really necessary, but otherwise we only discussed how to do it, not whether to do it or not.
Given that would allow us to remove a fair bit of code + and upgrade path + the follow-up issue, I think we should just do that here. It will mean updating some tests but that would need to happen anyway to remove the feature module.
Reverted for now - let's add the deprecated module documentation and update the test.
Yeah agreed we shouldn't block this on a test, it's a case that clearly falls under 'may not be needed' on https://www.drupal.org/about/core/policies/core-change-policies/core-gat... →
Committed/pushed to 11.x, thanks!
Yeah removing the counts for now sounds fine - the total across all projects is listed on the organisation page anyway.
I think we could probably use route aliasing on the deprecated comment route as well, per https://www.drupal.org/node/3317784 → ?
This looks good. I was going to ask about where we're going to remove it from all the tests it's added to, but then I saw 📌 Update layout builder tests and set new config Active , we might want to split that issue into two, but can discuss on that issue.
Committed/pushed to 11.x, thanks!
We'll need to mark the module obsolete in Drupal 12 + add an update in the module to uninstall itself (same as other previously obsolete modules).
I think it might be good to split the test changes here out to a new issue since that could also happen in 11.x, so that the 12.x MR is just changing to obsolete?
I think this is in a good enough state to mark fixed now - if we need to make small changes to the wording we can figure that out in slack without an issue. If something big happens with PHP release cycles we'll need a new issue anyway.
This looks good, nice that we can get rid of @legacy-covers again pretty easily.
Committed/pushed to 11.x, thanks!
An alternative idea I had was to add the user cache context, so that cache context manager can filter out automatically based on optimization rules, but I need to figure out how much faster that actually is.
This seems definitely worth a try. It's a case where we get zero benefit from the cache not being per user, so there shouldn't be any downside, it's just a question of how much improvement there is.
Updated page at https://www.drupal.org/about/core/policies/core-change-policies/set-plat... → looks good to me. Only nit is we should change the 'statement' header to something else or remove it.
Haven't done a full review yet but +1 to copying the assets across. Does the MR so far handle deleting the assets and copying them again so they can be updated?
What are the concrete things that need to be done in this issue? As @quietone says, all four child issues are closed (fixed) and that is the only task in 'remaining tasks'.
@andypost suggested closing this issue eight years ago, in 2017.
You yourself, also in 2017 in #2323895-45: [Meta] Document format/content of various YML files → suggested that #2823463: Devise an extensible way to document extensible data structures → would cover anything remaining here. Do we need two issues open for one proposal?
So I can't think of a more appropriate status than "Postponed (maintainer needs more info)" because no-one has had any suggestions for additional things to do in this issue for eight years, and several things were done prior to that which covered the scope of the initial report.
The screenshot attached to the issue summary is for https://www.drupal.org/third-and-grove#org-page-issue-credit → , it shows 9 Drupal core core issue credits for the last year. I would except that number to be somewhere between 500 and 1,000.
When you click through to https://www.drupal.org/node/2373279/issue-credits → the last five issues on that page are Drupal core, which matches the 5 issues shown on the summary. This suggests that the summary is generated only from the first page of the pager.
e.g. page 8 of that view shows Drupal core credits from April https://www.drupal.org/node/2373279/issue-credits?page=8 →
I would expect the high level credit summary to list all credits for each project listed (and probably every project too), pretty sure this is how it used to work prior to the new credit system.
This looks good to me - encouraging that it's a bugfix for the current logic rather than reverting closer to how it was previously.
Didn't manually test it but looks like @msandoval did.
Committed/pushed to 2.0.x, thanks!
Committed/pushed to 11.x, thanks!
Yep let's do it - we can always revert if we think we've made them generally more reliable, or use #[Retry] when it's available.
I haven't been involved with this module since 2008 (!) - I made it solely to enable the same feature to be removed from Drupal core. It looks like there have been no commits from anyone since 2020 so to me it makes sense to accept the offer from @sagesolutions - I've granted you all permissions except for 'administer maintainers'.
Moving back to the project queue and 'fixed' but if that's the wrong place/status feel free to change.
Couple of minor points on the MR. Overall it's looking great.
The main overall question I have is how we'll eventually phase out the AJAX logic in here and more clearly delineate what's done for AJAX and what's done for HTMX and what's done for both, but I didn't have any good ideas when reviewing.
Committed/pushed to 11.x and cherry-pick (with a small amount of manual merge conflict resolution) to 11.2.x, thanks!
Committed/pushed to 11.x, thanks!
There is also the situation where the site template developer/creator needs to customize the theme, in which case a fork or runtime dependency is needed.
I think it's reasonable to expect site template authors to keep up with changes in Mercury - otherwise the theme will be permanently out of date (e.g. an old forked version) for new sites too.
If it is a runtime dependency then the user can't quite use it as a starterkit, since that can't flatten the dependency automatically. What do you think of that situation?
If the starterkit runs on both the subtheme and mercury, would that cover it? You'd need to run it twice and maybe update the dependency, but it doesn't seem that much different to running it once.
Committed/pushed to 11.x, thanks!
Committed/pushed to 11.x, thanks! Looking forwards to working with both of you on asset issues!
I think a follow-up is good, but yeah let's open it - 12.x will be open soon.
Still have some vague undefined reservations here, but hopefully those are unfounded, certainly nothing concrete any more - I think the class_exists() check resolves most of them anyway.
Committed/pushed to 11.x, thanks!
Committed/pushed to 11.x, thanks!
Committed/pushed to 11.x, thanks!
This removes the rsync and composer config from the schema without removing the config keys themselves - this is to support the hook_requirements() pointing to settings.php.
However it's not clear to me how someone is supposed to actually remove those config keys from a site?
We might need to add an upgrade path and skip the hook_requirements() - there should be one for when composer/rsync can't be found anyway.
Or if we otherwise want to keep that, how do people remove this from config?
Package manager isn't beta yet so manual steps are OK but a lot of sites have it installed via Drupal CMS now and we shouldn't cause them to have data integrity issues without an obvious way out.
For 3, maybe we start merging via GitLab?
We historically haven't been able to do this because it wasn't possible to customise the author on squashed commit messages, and probably other things too in the past, but iirc that's the last one, I think that still might be the case but haven't checked in the past month.
It would be _great_ to be able to use merge trains, but I feel like this issue is one of still several steps before we're able to do that.
Add another heading to the default issue body template called “Draft commit message”.
This feels like it would mean having to maintain both the issue credits themselves and the commit message separately, in different interfaces, and that they would get out of sync all the time. The commit message is also currently generated from the title, but that would also now be maintained in a separate place (or just disconnected entirely).
I'm not sure how a (more) collective solution could work technically. Currently both committers and subsystem maintainers can assign issue credit (hopefully this is the case with the new system) - when this is already done on a complex issue it definitely helps, and is a collective process.
However the commit message itself is automatically generated from the contribution credit and people's own accounts/metadata, so any manual tweaks like changing 'fix' to 'feat' or whatever have to be one by the actual person making the commit at commit time (not even people with commit access in general). With the new system I have to exclude myself from commit messages if the only thing I'm doing on an issue is committing it, then manually add the contribution credit for the commit - either doing it in the right order or manually editing the commit message to remove myself.
If we get into things like making sure people's email addresses are correct (I managed to end up with three in core commit logs - the no-reply d.o one I was trying to use, my personal email when I forgot to set no-reply on a new machine for a while, and a second no-reply one in a different format that I'm still not sure where it came from), then that is a lot of additional admin work that only the person being credited themselves actually knows about - and they may not have been on that issue for three years.
Issue credit is already the most time consuming part of the technical process of committing an issue (excluding review time, although one line commit typo fixes with 15 people on an issue, not even excluding that). So I'm very resistant to anything that increases that time, even by a little bit.
With #38 do we know whether gitlab's integration works without an email address or not? If not, I'd be fine with just a comma separated list, but it would be shame to change things yet again if it turns out gitlab can handle the format without email addresses.
I didn't realise that both the node and comment user picture rendering is behind a setting - although I think themes have to individually implement that setting and the UI for it?. That might mean ✨ [META] Expose Title and other base fields in Manage Display Active covers the rest here. Back to needs more info for now.
Agreed that trying to multiple render the users is no longer going to be useful with render caching.