This stuff really has rabbit holes inside rabbit holes.
The migrate EntityRevision destination plugin calls setDefaultRevision(FALSE) since 2014, i honestly don't know why. It loads an existing revision or the default revision, neither should have a reason to change whether or not it's currently a default revision, it should just update that revision. Lets see if removing that breaks any tests.
For the menu_ui stuff, the changes that only partially worked inside 📌 MenuTreeStorage shouldn't invalidate cache tags if the menu didn't change Active actually passed then here. The reason for that is again the partial and incorrect fix that was done in #2850022: Duplicating a non-default revision should produce a default revision for a newly created entity → , which allowed the partial and incorrect implementation in #3041326: Remove 'title' and 'description' from MenuSettingsConstraintValidator when used with content moderation by creating a draft of menu link content when a draft of it's parent content is created → . What we did is accept the changed value for isDefaultRevision() but then the return value ignored the current value and returned always TRUE if the entity is *currently* new. That means that it correctly saves the change as the default revision but then in post save, in \Drupal\menu_link_content\Entity\MenuLinkContent::postSave(), it now sees that it's actually supposed to be not a default revision, so it doesn't update the menu tree storage. Meaning, the menu link content entity exists as a regular enabled default revision, but it's just not in the menu tree. Saving it through any other means than the node form on a draft would then however resave it properly and it would become visible.
What I did there now is as suggested above, point those menu links to the /latest page, then they show up but only for users who are allowed to see the latest version. It's either that or not allowing to create new menu links on a draft. And I changed isDefaultRevision() to ignore a new value on a new entity, so that it doesn't change in the middle of saving.
I also removed the first check about new and not default revision, because both old and new implementation of isDefautRevision() do not allow for that to happen.
I see lots of new fails though, so I'll need to investigate where that comes from.
FWIW, I'm perfectly OK if we completely ignore Content Moderation (and Workspaces) here. The implementation is fairly well abstracted and we can easily do a follow up and start with some basic module exists checks to get the content moderation state.
> in the top bar in the case of content moderation could be outright inaccurate.
I think it's not that bad. It's a bit confusing when it's a pending draft, as it would show Unpublished then. But if you use workspaces, it gets more complicated anyway. It can be published in a non-live workspace and what not.
Also might be useful to look at https://www.drupal.org/project/moderation_sidebar → for some inspiration. It displays the current Moderation state (Draft / Published / Archived) usually and if you're on the published page and there is a pending draft, it says "Draft available". published states are green, non-published is red, an available draft is yellow. a pending draft is also red, I suppose that could be a different color.
Converted the 6 year old patch to a merge requests. Rebase was messy (surprise!) and I will definitely still need to do another pass to clean up remaining references to template_preprocess(), update deprecation and drupal_static() stuff, but tests *are* passing, that part was easier than expected, so setting to needs review for now.
> Claro prepends the "Edit" words into the title itself, in italics:
Technically, Drupal/the node module does that. If Gin is different then it explicitly overrides and customizes that. So we'll need to figure out who does what. navigation will eventually become the default/only/recommended solution in core but it won't be mandatory, so it might fall to that module to adjust things to fit with information in the top bar, but that's something to figure out once the design/goal is clear.
> In a followup, refactor ModuleHandler and ThemeHandler so they have a common base (they used to be the same thing after all) and provide OOP capability for hooks by themes. This likely needs
This belongs more in the meta issue than this, but keep in mind that there are good reasons why themes can't and must not be able to implement any hook. Currently, we only invoke the special theme-hooks (like form alter) for the active theme and its parents. And I think we shouldn't change that. And if we don't, then it's important that themes are not able to implement hooks that may be cached without considering the theme. Because it results in a completely unpredictable situation then. Think of a theme that implements hook_entity_type_alter(), the change will either be there or not depending on whether the cache clear happens in the frontend or the backend theme. The ability that themes can implement any hook was removed on purpose and we need to keep it like this.
berdir → changed the visibility of the branch 3495943-preprocess-combined-approach-with-skip to hidden.
This doesn't require any test changes anymore thanks to #3040878: Reset static entity cache on POST requests in tests → and the permission related problem now just results in a logged warning.
How much this really helps is hard to say, this method clearly shows up in blackfire profiles on pages that load and display a lot of translated entities (this includes menu links of which there can be dozens to hundreds), how much that is blown up to the overhead of blackfire is the tricky questions, but I'm confident that it does help.
The CR says it is from 11.2+, but I think the problem explained in #24 deserves an explicit mention that this attribute class can only be used if a module requires 11.2 since LegacyHook doesn't work for this.
This doesn't only introduce the specific Formalter attribute but also the concept of hook attribute classes for dynamic hooks? I'd suggest we add a second CR that focuses on how to define your own Hook attribute classes, specifically when it contains dynamic parts such as form ids or entity types.
> Nominally this looks good, but I'm concerned that this could prevent something from being added that we would want added and that would be almost impossible to detect.
HEAD already attempts to automatically block all functions that contain "preprocess" (being removed in the preprocess issue, but it's still there now) and update functions (but inconsistently), so I don't think this introduces anything new.
IMHO this is safer and more reliable than the attribute to stop processing in the middle of a file, as it only excludes functions that explicitly match patterns that will never be hooks.
Merge request is green and identical to the patch that was RTBC.
Not sure if we want to keep the explicit test coverage with the direct query, as with the issue that this is blocking, saving would result in an exception instead, we're kind of testing that issue, not this.
entity save always throws an exception when there is an error in saving. It however can return FALSE in case of a non-default revision, for which there is an existing issue, closing as duplicate of that: 🐛 Consider not returning FALSE when saving a non-default entity revision Needs work
> I wonder if we can somehow detect if you attempt to save the default revision as not-the-default revision and throw an exception then or something because that is not a valid thing to do.
good thinking, past-me! current-me discovered this again and opened an issue for it: 🐛 Disallow saving the current default revision as a non-default revision Active . This is blocking that now.
This was RTBC and somehow fell off the bandwagon, lets try to finalize it. Still applies, not sure why it was needs work, random fail maybe?
I think the only bit here that's not addressed yet is search, which is in \Drupal\node\Entity\Node::postSave() now, there are also node_comment hooks that update the search index.
Errors like this typically happens when doing an entity query on a non-existing field.
This is mixing terminology, is it now a content type or an entity type?
comment_entity_predelete(), which triggers the error, just does an entity query on entity id and entity type, that should absolutely not fail. It might be possible that your entity storage for comments is somehow messed up.
Is it really worth doing this when we are in the middle of changing all hook_requirements() implementations anyway?
.gitlab-ci.yml changes apart from temporarily enabling PHP_MAX are out of scope. Those performance optimizations are very much on purpose. the phpunit jobs took 30min+ here.
As commented in the other issue, the job output is full of entity_browser and search_api (and maybe other) nullable types errors, which makes it hard to verify that ther's nothing left in paragraphs.
While not a hard blocker, it would be helpful to make sure issues in those projects exist and are referenced.
> If it’s a view, it should show the views's title. But only on the front-end because it needs an edit button (not on the admin People page for example)
This already sounds like a non-trivial special case. views pages are not canonical entity pages, we'd need to hardcode frontend/backend logic. edit button is also a separate issue and depends on local tasks, views pages don't have local task for editing at the moment, so that would also require custom views specific logic there.
My suggestion would be to just focus on regular entity routes here.
I guess one question is whether canonical pages and edit/delete pages should both just show the title or something else. I feel like Edit %title would duplicate the page title, the screenshot in https://git.drupalcode.org/project/drupal/-/merge_requests/10417#note_43... shows that perfectly. Is that really useful? The same can be said about the frontend, but on the frontend, the title is usuallly further away and not always obvious, depending on the design (hero elements, hidden titles, ...
Created a merge requests, doesn't quite fit the actual issue, can also move it somewhere.
Created a MR with a quick fix, but again, I honestly don't understand what the goal here is and I think that could be simplified a lot if it's internal to OpenTelemetryLogs
And disabling the deduplication feature also causes a fatal error, because it calls $this->logger->onTerminate(); but doesn't check if that actually is an OpentelemetryLogEnhancer object.
How about this idea as a compromise?
Require an explicit configuration of the endpoint, if the field is empty then skip tracing/logging/whatever, but ship it with localhost as default configuration?
if ($this->settings->get(OpentelemetryService::SETTING_ENDPOINT)) {
$this->logger = $this->loggerProvider->getLogger(
$this->settings->get(OpentelemetryService::SETTING_SERVICE_NAME) ?? OpentelemetryService::SERVICE_NAME_FALLBACK
);
}
else {
$this->logger = NoopLogger::getInstance();
}
There might be better ways to set that up, but it wasn't obvious to me.
And make the disable checkbox in the UI clearer: "Disable tracing"
Or, make the disable checkbox apply to everything and then check for that instead of the endpoint. But I actually see use cases for having logging enabled but not tracing. Especially since I won't be able to have a local opentelemetry collector to batch data together, I'm still unsure how much data I really want to collect and process.
> is expected, it just warns you that the endpoint is not available.
This is similar to issues I opened, which changed it from an error to a warning, but I think that's still problematic.
I think there really needs to be a complete off switch without having to uninstall the modules, for example for non-production/local development environments, CI and also initial install.
There is the disable checkbox, but currently it's only respected for traces (without explaining that) and not logs.
Drupal modules can include a link to their configuration site which is shown after install now by default in drush, there could be a runtime requirements hook to show an error if the connection fails.
It used to, but the new implementation no longer needs to load.
> What's the reasoning for the partial namespace import for Drush command attributes? Why not import the whole thing, or alias them if there's risk of a name collision?
I'd say because the attribute name themself are very generic (Command, Help, Usage, ...) and CLI gives them context. Technically it works correctly, but it's just against the common usage that drush itself has established, so maybe it just needs an exception for that.
I'm OK with including this as an intermediate fix until the core issue is resolved and I guess it could in theory still happen.
Please provide this as a merge request and include a svg source file so it can be edited.
I'm a bit unsure if the text on there is too small for a logo. See how small for example the pathauto logo on the project page is: https://www.drupal.org/project/pathauto → .
maybe project browser is a bit bigger, but even then, is it really possible to make out those menu links? Possibly it can be simplified a bit?
Thanks, merged.
Fixed a few bugs in the tests, now there's a sensible amount of fails.
don't really understand what Drupal.KernelTests.Core.Entity.RevisionableContentEntityBaseTest is doing, but I don't think what it does is sensible.
\Drupal\KernelTests\Core\Entity\EntityDuplicateTest::testDuplicateNonDefaultRevision() is 🐛 ContentEntityBase::createDuplicate() should reset default revision flag Needs work , forgot about that, that was RTBC but then somehow it got lost once it was set to needs work. Probably a blocker for this.
\Drupal\Tests\content_moderation\Kernel\ContentModerationSyncingTest::testUpdatingPreviousRevisionDuringSync() is tricky, it's resaving an old revision that was default but no longer is. I'll need to double check if we have a way to detect the difference between current default revision and previous default revision. The migrate tests might be similar, not sure yet.
\Drupal\Tests\menu_ui\Functional\MenuUiContentModerationTest::testMenuUiWithPendingRevisions is the fail I expected. I have one idea that we can try, but it will complicate things a bit and it depends on content_moderation specifically. The idea is that we'd create that menu link to the latest route, where access is denied if you don't have access to preview the draft, and change it back. But it complicates things, we always need to look for that route, or at least if it's a non-default-revision node being edited and update the target on publish.
I know there were other issues about this: 📌 Improve consistency in EntityFormInterface::save() implementations Needs work
Forgot about this issue, see m recent comments in 🐛 Remove return value from EntityFormInterface::save Needs work
Created a merge request, this should fail.
Instead of a filesize, it's also possible to set size restrictions and it will automatically resize it. That said, how big the icon is displayed can easily be adjusted by custom sizes, ours are considerably larger than the default.
Created a basic merge request, will need an update with the change record if others agree.
Created a merge requests, seems to be working pretty well.
Cross-reference: 📌 Deprecate CacheBackendInterface::invalidateAll() Active .
Working on 📌 Add caching to ConfigurableLanguageManager::getLanguages() Active reminded me about this, as that issue adds a very early cache tag. The first in fact if page cache isn't a hit.
I still think this is interesting, but I think it's not quite so straight-forward, especially the timing of when we do this load.
Tests are all against umami /en/recipes/super-easy-vegetarian-pasta-bake
The best case scenario is that we have an internal page cache hit. That means exactly one cache tag lookup.
["config:block_list","config:block.block.umami_search","config:block.block.umami_languageswitcher","config:configurable_language_list","config:block.block.umami_account_menu","config:block.block.umami_page_title","config:block.block.umami_messages","config:block.block.umami_help","config:block.block.umami_main_menu","config:block.block.umami_branding","config:block.block.umami_local_tasks","config:block.block.umami_footer","config:block.block.umami_footer_promo","block_content:2","config:block.block.umami_content","config:block.block.umami_breadcrumbs","config:block.block.umami_views_block__articles_aside_block_1","node:3","config:block.block.umami_views_block__promoted_items_block_1","config:block.block.umami_banner_home","config:block.block.umami_banner_recipes","config:block.block.umami_views_block__recipe_collections_block","config:block.block.umami_disclaimer","block_content:1","block_view","config:filter.format.basic_html","config:system.menu.footer","block_content_view","config:core.entity_view_display.block_content.footer_promo_block.default","media_view","media:19","config:image.style.medium_8_7","file:37","config:core.entity_view_display.media.image.medium_8_7","config:views.view.recipe_collections","taxonomy_term_list","taxonomy_term:16","taxonomy_term:15","taxonomy_term:14","taxonomy_term:13","taxonomy_term:12","taxonomy_term:11","taxonomy_term:10","taxonomy_term:9","taxonomy_term:8","taxonomy_term:7","taxonomy_term:6","taxonomy_term:5","taxonomy_term:4","taxonomy_term:3","taxonomy_term:2","taxonomy_term:1","node_view","config:core.entity_view_display.node.recipe.full","config:views.view.related_recipes","node_list","node:1","node:6","node:7","node:10","media:1","config:responsive_image.styles.3_2_image","config:image.style.large_3_2_768x512","config:image.style.large_3_2_2x","config:image.style.medium_3_2_2x","config:image.style.medium_3_2_600x400","config:core.entity_view_display.media.image.responsive_3x2","config:core.entity_view_display.node.recipe.card","user:6","media:6","media:7","media:21","media:3","taxonomy_term:21","taxonomy_term:28","taxonomy_term:31","local_task","config:workflows.workflow.editorial","config:system.menu.main","node:20","config:system.site","config:system.menu.account","CACHE_MISS_IF_UNCACHEABLE_HTTP_METHOD:form","rendered","http_response","config:user.role.anonymous"]
One single lookup with all the tags. We neither can nor want to preload them all, because they will be different on every page. In other words, trying to preload common ones before this will actually expand it from one to two queries.
Second best is a dynamic page cache hit:
/en/recipes/super-easy-vegetarian-pasta-bake
["config:user.role.authenticated","config:user.role.administrator","access_policies"]
["route_match"]
["entity_types"]
["node_values","entity_field_info"]
["config:block_list","config:block.block.umami_page_title","config:block.block.umami_messages","config:block.block.umami_help","config:block.block.umami_content","config:block.block.umami_views_block__articles_aside_block_1","node:3","config:block.block.umami_views_block__promoted_items_block_1","config:block.block.umami_banner_home","config:block.block.umami_banner_recipes","config:block.block.umami_search","config:block.block.umami_languageswitcher","config:configurable_language_list","config:block.block.umami_account_menu","config:block.block.umami_main_menu","config:block.block.umami_branding","config:block.block.umami_local_tasks","config:block.block.umami_footer","config:block.block.umami_footer_promo","block_content:2","config:block.block.umami_breadcrumbs","config:block.block.umami_views_block__recipe_collections_block","config:block.block.umami_disclaimer","block_content:1","block_view","config:filter.format.basic_html","config:system.menu.footer","block_content_view","config:core.entity_view_display.block_content.footer_promo_block.default","media_view","media:19","config:image.style.medium_8_7","file:37","config:core.entity_view_display.media.image.medium_8_7","config:views.view.recipe_collections","taxonomy_term_list","taxonomy_term:16","taxonomy_term:15","taxonomy_term:14","taxonomy_term:13","taxonomy_term:12","taxonomy_term:11","taxonomy_term:10","taxonomy_term:9","taxonomy_term:8","taxonomy_term:7","taxonomy_term:6","taxonomy_term:5","taxonomy_term:4","taxonomy_term:3","taxonomy_term:2","taxonomy_term:1","node_view","config:core.entity_view_display.node.recipe.full","config:views.view.related_recipes","node_list","node:1","node:6","node:7","node:10","media:1","config:responsive_image.styles.3_2_image","config:image.style.large_3_2_768x512","config:image.style.large_3_2_2x","config:image.style.medium_3_2_2x","config:image.style.medium_3_2_600x400","config:core.entity_view_display.media.image.responsive_3x2","config:core.entity_view_display.node.recipe.card","user:6","media:6","media:7","media:21","media:3","taxonomy_term:21","taxonomy_term:28","taxonomy_term:31","local_task","config:workflows.workflow.editorial","config:system.menu.main","config:system.site","config:system.menu.account","CACHE_MISS_IF_UNCACHEABLE_HTTP_METHOD:form","config:system.theme","config:system.menu.admin","rendered","http_response"]
["library_info"]
["routes"]
["config:shortcut.set.default"]
Multiple queries that are very stable and easy to optimize, but the dynamic page cache one itself is not, same as above. And the access policies might *look* easy, but it contains the tags of all roles of the current user. See below point 3.
Umami has history, so also triggers a second request:
["config:user.role.authenticated","config:user.role.administrator","access_policies"]
["route_match"]
["entity_types"]
["node_values","entity_field_info"]
["routes"]
Same page, but disabled dynamic page cache, with render cache hits.
["config:user.role.authenticated","config:user.role.administrator","access_policies"]
["route_match"]
["entity_types"]
["node_values","entity_field_info"]
["routes"]
["node_view","node:3","config:core.entity_view_display.node.recipe.full","config:views.view.related_recipes","node_list","node:1","node:6","node:7","node:10","media_view","media:1","config:responsive_image.styles.3_2_image","config:image.style.large_3_2_768x512","config:image.style.large_3_2_2x","config:image.style.medium_3_2_2x","config:image.style.medium_3_2_600x400","config:core.entity_view_display.media.image.responsive_3x2","config:core.entity_view_display.node.recipe.card","user:6","media:6","media:7","media:21","config:filter.format.basic_html","media:3","taxonomy_term:2","taxonomy_term:21","taxonomy_term:28","taxonomy_term:31","rendered"]
["config:views.view.recipe_collections"]
["block_content_values"]
["config:system.theme","config:system.menu.admin"]
["block_view","config:block.block.umami_search","CACHE_MISS_IF_UNCACHEABLE_HTTP_METHOD:form"]
["config:system.menu.account","config:system.menu.main","config:system.menu.footer"]
{"1":"config:block.block.umami_account_menu"}
{"1":"config:block.block.umami_branding","2":"config:system.site"}
{"1":"config:block.block.umami_main_menu"}
{"1":"config:block.block.umami_messages"}
{"1":"config:block.block.umami_help"}
{"1":"config:block.block.umami_local_tasks","2":"local_task","5":"config:workflows.workflow.editorial"}
{"1":"config:block.block.umami_breadcrumbs"}
{"1":"config:block.block.umami_views_block__recipe_collections_block","3":"taxonomy_term_list","4":"taxonomy_term:16","5":"taxonomy_term:15","6":"taxonomy_term:14","7":"taxonomy_term:13","8":"taxonomy_term:12","9":"taxonomy_term:11","10":"taxonomy_term:10","11":"taxonomy_term:9","12":"taxonomy_term:8","13":"taxonomy_term:7","14":"taxonomy_term:6","15":"taxonomy_term:5","16":"taxonomy_term:4","17":"taxonomy_term:3","19":"taxonomy_term:1"}
{"1":"config:block.block.umami_footer_promo","2":"block_content_view","3":"block_content:2","4":"config:core.entity_view_display.block_content.footer_promo_block.default","6":"media:19","7":"config:image.style.medium_8_7","8":"file:37","9":"config:core.entity_view_display.media.image.medium_8_7"}
{"1":"config:block.block.umami_footer"}
{"1":"config:block.block.umami_disclaimer"}
["library_info"]
["config:shortcut.set.default"]
However, a scenario with d(ynamic) page cache miss due to an invalidated cache tag is I guess also very common, where that will basically act as a bulk load for many of the following cache tags that will be checked again.
To me, that means two things:
1. I think it should not be an internal thing in the checksum trait but an external request subscriber that runs very early but after page cache. It doesn't matter too much if it's on a page cache miss because then most will already be filtered out from the static cache.
2. I think we should avoid making a first iteration too complicated. We can introduce the concept, with a few examples that remove a handful of fixed cache tags that the vast majority of sites use. A contrib module could always experiment with more, I also still think supporting extra cache tags through Settings::get() would be an easy way to optimize specific sites.
3. I still see a lot of potential to optimize away overlapping and unnecessary dynamic cache tags. Why do we have routes and route_match, do we really need both? I have an issue open about the block ones, that turned out to be not so trivial but it has a lot of potential. For access policies, what if we just invalidate the access_policies cache tag if any role is saved, instead of adding a dynamic amount of cache tags? It's always a tradeoff between performance and simplicity, and invalidations vs. lookups.
This also reminded me about the current redis issue with the last write, which is quite inefficient, created 📌 Optimize bin cache tags and last write timetamp Active . Which in turn made me wonder if fast chained backends should use cache tags instead of a regular cache get on the fast backend? They now require cache tag support anyway and it would mostly avoid the redis problem and could be combined with the preloading done here.
Performance stuff is always _such_ a rabbit hole ;)
I think there's actually one more lookup for each last write timestamp, the cache tag IIRC is for invalidateAll() which is almost never used, and there is a separate delete flag that's not a tag.
One question is whether we can avoid the cache tag. We have a variable amount of cache keys, but the amount of variations is limited, we might be able to do a bulk delete this. Not sure yet.
To get the tests to pass, this required a single manual cache clear in a test that's explicitly messing with the default language service in the container on the fly. I think that's fair.
This saves an early bootstrap query on every single multilingual site requests that isn't an internal page cache hit. When using redis, that means the amount of queries go down from 6 to 5 on a dynamic page cache hit:
What's strange is that \Drupal\Tests\demo_umami\FunctionalJavascript\OpenTelemetryAuthenticatedPerformanceTest::testFrontPageAuthenticatedWarmCache isn't failing, because that's exactly the query that should go away. Ah, I think those tests don't run by default. I also wasn't able to run it manually, go this error:
This job is stuck because of one of the following problems. There are no active runners online, no runners for the protected branch , or no runners that match all of the job's tags: performance-test-runner
Locally, it 's failing, but not only because that query is gone, also because I'm getting a race condition on automated_cron. Reliably got this fail 3 times:
1) Drupal\Tests\demo_umami\FunctionalJavascript\OpenTelemetryAuthenticatedPerformanceTest::testFrontPageAuthenticatedWarmCache
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
0 => 'SELECT "session" FROM "sessions" WHERE "sid" = "SESSION_ID" LIMIT 0, 1',
1 => 'SELECT * FROM "users_field_data" "u" WHERE "u"."uid" = "10" AND "u"."default_langcode" = 1',
2 => 'SELECT "roles_target_id" FROM "user__roles" WHERE "entity_id" = "10"',
- 3 => 'SELECT "config"."name" AS "name" FROM "config" "config" WHERE ("collection" = "") AND ("name" LIKE "language.entity.%" ESCAPE '\\') ORDER BY "collection" ASC, "name" ASC',
+ 3 => 'INSERT INTO "semaphore" ("name", "value", "expire") VALUES ("cron", "LOCK_ID", "EXPIRE")',
+ 4 => 'SELECT "expire", "value" FROM "semaphore" WHERE "name" = "cron"',
+ 5 => 'INSERT INTO "watchdog" ("uid", "type", "message", "variables", "severity", "link", "location", "referer", "hostname", "timestamp") VALUES (0, "cron", "Attempting to re-run cron while it is already running.", "WATCHDOG_DATA", 4, "", "LOCATION", "REFERER", "CLIENT_IP", "TIMESTAMP")',
]
I think the non-cron-on-installer change really did a number on this test. Others maybe too, didn't try to run them. The umami based test is special because it's first cron run is really slow. There's a lot to index there.
I tried a number of things such as the wait for terminate event (adds another query for the lock and wasn't reliable enough either), explicit lock wait(), sleep(). The one thing that seems to reliably help is explicitly running cron before the first web request. Maybe the performance tests should just run without that module?
So I spent quite some time looking into that test fail.
And it's a mess. I don't know the people in #3041326: Remove 'title' and 'description' from MenuSettingsConstraintValidator when used with content moderation by creating a draft of menu link content when a draft of it's parent content is created → were thinking with that change, that doesn't work at all. (hint: my name is in the commit credits. well done past me, truly well done). It resaves default-revisions as non-default and even attempts to create new non-default-revision entities. The way MenuLinkContent::postSave() works does in fact result in not immediately pushing that change to the menu tree storage, but it's still half-stored (only in the revision tables and not the data table) and any future resave will result in it being updated.
I was able to fix the first fail by switching menu_ui to saving a non-default revision as a new revision. But then it got even weirder, with the expectation that you can create a new menu link during creating a non-default node draft revision. It's one thing to add a menu link for an initially unpublished but default revision node, but not a pending draft.
The reason it becomes visible with this change is that this new-and-kinda-non-default-revision menu link is actually the default revision again when being loaded. So I'm unable to detect a change since the node I'm saving also is the default revision now.
I don't see how I can make this work properly, I think we need to remove and disallow that possibility. I could probably pull some shenanigans, like saving the menu link twice, so we actually get a draft, but that just feels so wrong.
Also. The fact that you can resave an existing default revision of an entity as a non-default revision is IMHO a critical entity bug, will see if there's an issue about that one.
I created a first merge request, this still needs to be properly tested on our side.
The logic is a bit convoluted now due all the if/else conditions, it might make sense to introduce two methods to abstract this, wanted to keep the changes minimal for now.
At first I wasn't sure if the adjustment types setting works with order items then, but I think it actually works very well, all the API's we need allow to provide adjustment types, just have to be careful about when it ends up being empty.
Thanks, closing this as a support request then, I also commented there.
I don't think this approach is correct, we don't know if there are any fields that aren't handled by TMGMT and should in fact be kept, things that are manually translated. As shown in the failing test, that is, per comment there, a deliberate decision.
So I think it's unlikely that this will be committed.
Also, per 🐛 Error when creating new translation Active this seems to fail on the latest version.
Might be some strange data on your site? Will be difficult to do something about this without a way to reproduce, aka a failing test.
One concern I have with this is that the placeholdering will cause layout shift. Sites with very large/complex menus that currently take a considerable amount of the total render time might run into this.
Did a bit of manual testing on a client project, and it doesn't seem as bad as I expected, but if I enable placeholdering for all blocks for example it's clearly visible.
One idea I had is that we introduce a new flag next to create_placeholder that's a bit similar to loading=eager/lazy on images. For things that are above the fold, we can define to placeholder it but still render within the initial response. if I understand correctly then this wouldn't actually bring the benefits you're looking for yet and we'd need the followup. We could then include that by default for the blocks we change for now and contrib could extend and make that configurable per block or something. Things like the footer navigation are then very obvious targets to render later, while the main navigation probably should be there from the start.
So, I did two things now.
1. A new merge request that implements my suggestion of not saving menu links, which is similar to the linked issue and merge request in #16. I did explicit checks because I'm not sure if hasTransationChanges() behaves correct on untranslatable fields like the menu name.
2. I created new contrib project menu cache: https://www.drupal.org/project/menu_cache → . This is a proof of concept that removes node cache tags from menu blocks (this was not trivial to figure out, needed to add a pre_render callback in hook_block_view_alter() and adds explicit invalidation of the existing menu cache tag if the node is published or unpublished. It's very much a proof of concept, but it has a fairly extensive test that shows that it works for the scenario that it supports when combined with the new MR here.
I did not yet look into combining it with token module and the apparent separate save.
Yes, that's the problem with accessing the property, it auto-initializes, so that condition is always true if it can be loaded.
I don't think your change is correct either. getValue() will never return computed values, so the result is the same as HEAD.
I don't think it is possible at all to do this, because even accessing the EntityReference property directly with get('entity') there simply is no API to detect whether it has been set or not. There is no public method to access $this->target and \Drupal\Core\Entity\Plugin\DataType\EntityReference::getTarget() always loads it if not set, that's the same as ->entity.
That's why \Drupal\Core\Field\Plugin\Field\FieldFormatter\EntityReferenceFormatterBase::prepareView + \Drupal\Core\Field\Plugin\Field\FieldFormatter\EntityReferenceFormatterBase::getEntitiesToView() use the magic _loaded property to track that.
So to change this, we'd first need to extend the underlying entity reference property to add a method to check if the refernce is loaded or not.
> No need to postpone this, as you mentioned this needs planning which this issue is for.
I've seen a fair share of issues being picked up that by less experienced contributers to work on it, but doesn't matter too much.
On the Hooks classes, that's not quite how I would group things. For example InfoAlterHooks do all alter things, but the things they alter are very different. Somehow you've grouped many of them by the suffix, to me the prefix/first word is typically the primary grouping factor, as suggested in the cleanup meta issue (of which this should probably be a child issue?). To me, the goal should be that I can look at a list of hook classes and be able to make an assumption about which one contains e.g. hook_entity_type_alter(). Right now, I'd have no idea.
entity_bundle_info_alter(), entity_base_field_info(), entity_extra_field_info(), entity_type_alter() to me belong in the same group, they are altering different aspects of of entity type definitions. That some are called type alter and others info alter and some aren't alters is just a bit inconsistent naming. I'd also put hook_field_info_alter() in the same group I think. entity and field API's are closely related. How to call that depends a bit on what we'd do with the rest of the entity hooks. In most modules, I'd probably put all entity related hooks in a single Hooks class and just go with EntityHooks. This module has a lot of them, so I seen argument of splitting that up into definition and runtime hooks, so I'd probably go with EntityDefinitionHooks for this one?
Despite not starting with entity, language_fallback_candidates_entity_view_alter() is also an entity system hook, so that could be in EntityOperations too if we want to go that (not 100% sure I like it, but not sure that just EntityHooks is better). I can also seen an argument that language_fallback_candidates_entity_view_alter, the language_content_settings_* and language_types_info_alter() could be in a LanguageHooks. Unsure about this one. some of those operate on an entity and some don't, but pretty much all the hooks do. If you hook at page_attachments_alter(), that is also 100% about entity operation stuff.
views_data_alter just one hook, but for most modules, we've already established that we have a ViewsHooks class, as they were migrated from views.inc hook group, so I think it makes sense to adopt that here as well.
The leftovers possibly in ContentTranslationHooks?
Thoughts?
I knew there was another one. This is fixed now in 11.x by 📌 Define 'original' as property on the entity object Needs work
This is for the file usage table, which has an type and id column. That's not a relationship and can't be expressed as a relationship because every column can be a different entity type.
This also fixed a PHP 8.4 issue, there's still one issue being reported from entity_browser.
Gitlab CI reports issues too.
This is way too much for a single issue. css, js, phpstan and coding standards should each be split into separate issue as a start.
Setting to postponed to prevent someone from picking this up for now. I think it's too early? Should at least wait until preprocess and module implements alter is done, for the .module/admin.inc cleanup part?
My gut reaction is that yes, this is too much for a single issue. admin.inc alone is 400 loc that will need to be reviewed somehow for things like DI and what one.
Some functions like content_translation_translate_access are related to several open issue about improving and fixing translation access. That's one deep rabbit hole. I'm not sure if I just want to move it to a service just so it's there.
> I think the acquia connector is one that would need to run in the update alter.
Actually not, no. system_requirements() already deals with runtime update, again to not prevent you from updating:
$requirements['php']['severity'] = ($phase === 'runtime') ? REQUIREMENT_ERROR : REQUIREMENT_WARNING;
It will never be ERROR on phase update, only on runtime.
Also, this will all only be relevant again in 2028. The logic around this is that we switch to a runtime error if the PHP version is EOL, per \Drupal\Core\Utility\PhpRequirements::$phpEolDates, that is 2027-12-31 for PHP 8.3. Nobody will see a requirements error for the next 3 years.
But either way, I'm OK with keeping the alter hooks just in case, probably not worth to spend too much time on discussing that (oops, too late).
> 16 modules had ben marked as fully converted, I changed the parameter to false
Yes, but we can only toggle core modules, not contrib that might have already picked this up.
As of today, 4 modules have added the flag: https://git.drupalcode.org/search?group_id=2&repository_ref=&scope=blobs.... That is exactly 4 more than I expected would do so ;) Based on a quick check, none of them would break because of this. But it takes another 5 or so month until 11.2 comes out and more will add it and then possibly break.
Technically, it *is* a BC break. Whether or not we care about that is for a core/framework/release maintainer to decide. IMHO we should do one of the following two options:
1. We play it safe and rename the parameter. Update the change record and state that modules should probably just wait with implementing that until 11.2 or if they care enough, add the parameter for 11.1 and then also add the one for 11.2 once they updated preprocess (if they have that). We lose the optimization on a handful of modules.
2. We keep the name and just toggle it for core as you did where necessary. We risk breaking a few modules. In this case, we should IMHO update the change record _now_ to state that if your module has preprocess hooks, then you should either not use it or you will need to touch it again and fix it for 11.2 (or whenever this issue lands. The LegacyHook approach should work fine on 11.1 because it doesn't parse preprocess functions anyway, LegacyHook or not, and they don't go through the ModuleHandler. there.
Referencing some template_preprocess related issues that we might want to revist/re-evaluate afte this lands:
📌
Replace template_preprocess() and possibly other things with Twig global variables/functions
Active
#2340341: Move template_preprocess, _template_preprocess_default_variables into services →
Didn't review the new MR yet.
> would we want to preserve the ability to alter it if we move it to a service?
This was specific to the idea of moving system requirements stuff out of the hook, but I think it's a question that's worth asking in general.
Use cases for altering requirements seem rather rare. I can maybe see some for runtime hooks, but update?
Looking at https://git.drupalcode.org/search?group_id=2&scope=blobs&search=%22Imple..., there are exactly four in contrib, although possibly quite a bit more in custom code where people use it to remove undesired things?
Maybe we could skip adding the alter for update. On the other hand, it costs us little to have the hook and if people do weird stuff with it then that's on them?
I think most importantly, it also properly excludes update functions from being picked up, which is currently not working reliably when you have submodules with update functions.
Yes, it doesn't replace the need for StopProceduralScan, but it lessens the memory overhead when modules don't use it, and most will not because it's not trivial to use and quite tedious.
FWIW, StopProceduralScan is also mostly a memory optimization. The parsing is done, the only thing we save on is running the regex on the remaining functions in the file and then also not adding them to the list of possibly legacy hooks.
Sorry for the notification spam. ideas keep popping up.
We could add a trait like EntityFormDefaultSaveTrait or something. The actions form can check if that's present and if yes, use the new save() method, if not it doesn't and the current save() is changed to trigger a deprecation message. In D12/13, we include that trait by default and remove the old method.
Submitted too early.
I'm aware that the proposal has a way larger scope then this issue. And there are several open related issues about it. Im also not sure how we'd deprecate and switch since it's a defined form callback and subclasses rely on it being called? A flag to opt-in to a new method that we'd deprecate again in D12 where we switch the default? A bit like the show revision ui flag we have on entity forms.
> Re the rest of #12 - yes it's sort of bogus, but it is used in places to determine if it was a new or existing entity (as per the doc block).
Yes. There are two things here though.
As a form callback, returning a value is meaningless. It's not used, form callbacks don't have a return value.
But, the problem are $return = parent::save() calls. Like for example core/modules/media/src/MediaTypeForm.php. If we remove the return in EntityForm::save(), that will break.
I'm not exactly sure how we should deprecate that. We could start with just removing the documentation for it and creating a change record, but I don't think we can actually trigger a deprecation.
Instead of using the save return value, form classes can do this:
$is_new = $this->entity->isNew();
parent::save();
if ($is_new) {
// created message.
}
else {
// updated message.
}
Easier and doesn't need those strange constants.
The current save() method is a trivial oneliner. I'm not sure why we even have a separate submitForm() and save() submit callback. Maybe to allow to customize it. I'd love to improve that in general. We have a trillion of slightly different save implementations, some log, some display a message, some just one, some different depending on new/updated. Not a single one of them catches exceptions to avoid a fatal error if something goes wrong during saving. If we change something, maybe we could deprecate the whole method and replace it with something that has sensible default logic. mabe a similar pattern as confirm forms to customize the messages if necessary?
> Should the return value of EntityInterface::save() and EntityStorageInterface::save() also be removed? And if so, should we create a separate issue for that or can we deal with that here?
The change in the current MR is wrong. No, we don't want to change EntityInterface::save() nor EntityStorageInterface::save(). *only* EntityForm::save(). The MR should just change the form class, *not* the storage class.
Closing as duplicate of 🐛 Remove return value from EntityFormInterface::save Needs work , see discussion there.
I also commented in 🐛 ContentTranslationManager::isEnabled() can load/create the same config entity many times Active about this.
The dependency stuff is interesting. Do we have any other use case where the dependency is based on a certain *value* of another config entity as opposed to its existence.
As mentioned over there, combined with the hook, it results in two parallel cleanup processes of role permissions.
@alexpott: Note that this is an edge case that is not covered by your issue. If we enable static caching, then the PermissionsProviders sees the changed entity that already has the third party setting removed and won't define the permission for it then. But it hasn't (yet) gone through save/update hook, so ContentTranslationHooks can't remove the permission before this happens.
I'm also unsure what will happen exactly in the scenario where both roles and language content entities are updated and saved in parallel and then ContentTranslationHooks will *also* load and save the role. Because role config entities already are statically cached, the result might be correct. Still, very complex and scary stuff.
I think to avoid side effects due to changing config entities in memory, we should look into option b that I mentioned:
b) Clone entities that we're about to change in \Drupal\Core\Config\ConfigManager::getConfigEntitiesToChangeOnDependencyRemoval so that we alter things that others might depend on in their logic.
We'll likely run into more issues as we enable static caching on more config entities.
> Yes because the module now require the contrib Action module which is not compatible with Drupal 10.2 and below.
There are two things here.
The .info.yml notation of "project:module". The project part at runtime is completely ignored. "drupal:action" and "action:action" for Drupal is really just "action". You could write "foobar:action" and Drupal is still happy. Modules don't actually have a project group/namespace. The prefix is just for drupal.org to translate it into composer dependencies.
The other thing is the composer requirement. I was wrong on that. drupal/core used to have replace statements for all modules it provides, but that was reduced to just components in Drupal 9, see composer.json in 8.9.x: https://git.drupalcode.org/project/drupal/-/blob/8.9.x/core/composer.jso....
Technically you could add your own replace section to your project composer.json, but since 10.1 and older are long EOL anyway, I agree it's easier to just require 10.2 and move on.
I don't think it's a BC break though (don't think that's a valid issue tag either way), updating required versions of dependencies is allowed in a minor update.
There's already 📌 Move the Save button into the Top Bar Active , I don't think this needs any special handling in regards to edit as long as the edit only shows up on the canonical route.
> 1. You can implement both hooks with a single method, that is sufficient for most simple requirements implementations.
FWIW, if we except this to be the default then we could also keep a single hook with an argument. We could possibly even keep the name, we'd just stop calling the new hook with phase install.
But I think the blocking thing with update is worth to separate to not make mistakes around that. Especially if improve default support for explicitly checking extensions/composer requirements, update should probably focus on things that are _actually_ relevant when updating, to not overload you with irrelevant warnings and information. So I think that's a good argument to have two separate hooks and say only implement update_requirements if it's actually relevant to the update process.
layout_discovery_requirements() is also an interesting case for hook_install(), it's a conflict-check and not a dependency check. Preventing install if another module is around. Same for workspaces. So if we want to support that we'd need another new .info.yml thing.
Searching for \$phase ===? 'update' finds only a single case that explicitly checks for update outside of system module, and that's valid, google_tag that checks things around google_analytics.
For me that confirms that two separate hooks (and a separate solution for install) are likely the most sensible way forward. runtime requirements should cover 80-90% of the cases outside of system_requirements() and probably 90% if we support install time extension/composer dependency checks. And for system we can for example start with option 3 above.
> but also something like $entity->view I would assume was added 'just in case it will be useful' in about 2009 and we should deprecate it with no replacement. Would be good to ay least open issues for the spurious looking ones.
We actually have a valid and for us important use case for this. We have a views display plugin that allows to control certain options on customizing displayed teasers, in this case a customized block display that is then used through paragraphs/block_field so editors can display a list of content and control how it looks without requiring 10 different view modes.
Same with _referringItem in combination with entity reference fields.
Both are tricky to use because it's on you to adjust cache keys of the rendered nodes, but it's possible.
That said, views uses it to provide theme suggestions for that (without supporting caching properly) and that we should deprecate: 🐛 [PP-1] Node/comment views-based theme suggestions have no cacheability metadata Postponed , but it's blocked on #3159050: Allow deprecating theme suggestions → and that got stuck. So many issues...
You can see some of the more frequent cases that we have for this in the current __get()/__set() MR where I added an ignore list for them just so I could get a better overview of other remaining bits: https://git.drupalcode.org/project/drupal/-/merge_requests/10629/diffs#7...
So we have things like:
* pass_raw/passRaw, used by drupalCreateUser/drupalLogin on the user entity
* _skipProtectedUserFieldConstraint again for user entities, actually used by user module itself. Explicitly "internal" but also kind of an API.
* _restSubmittedFields, which is a workaround for not having an entity factory (to create an entity without default values), which is another chain of issues a decade+ old
* _initialPublished, which I think is workspaces
* in_preview/preview/preview_view_mode, as mentioned already
* book/rdf, which are removed from core by now
* _serviceId, is also replaced with a new solution now I think
A slightly different version of this but related issue that's a problem for config entities and the main reason we had to make them #[AllowDynamicProperties] is \Drupal\Core\Entity\EntityForm::copyFormValuesToEntity(). That just slaps all form state values into the entity using set() (content entities subclass this and check with hasField()). For config entities, set() behaves like __set() for content entities. BC around that will be fun. And there's some content entity deprecations triggered there on form submission that I still have to track down.
In my previous comment, I didn't really mean to point out a problem with removing the phase, more the opposite, by saying just because system_requirements() might need it, that might not be a reason to keep it.
However, I realized something that I believe is a hard blocker for removing the phase argument/separate hooks. Sure, the example in #42 wouldn't need to consider phase at all. However, the difference between update and runtime is that errors during phase update are *blocking* you from updating. We have many runtime errors that must _not_ block you from updating. Take a look at update_requirements() for example. Having (additional) available security updates must _not_ block you from running updates for the modules you already did update.
I don't see how that can possibly go away.
So yes, I think we either need to keep the argument or we need two hooks for update and runtime. Again, system_requirements() is a weird special case and we should split that up, but the new hook system already gives us a number of options to dealing with that, per #60:
1. You can implement both hooks with a single method, that is sufficient for most simple requirements implementations.
2. You can implement multiple hooks with varying combinations of one or both hook registrations.
3. You can even do the following, that might be easiest way for system_requirements() as a first step (and we can look into splitting up later) and it might also be an option for rector, with a comment to suggest to simplify it as feasible:
class SystemRequirementsHooks {
#[Hook('update_requirements')]
function updateRequirements() {
return $this->requirements('update');
}
#[Hook('runtime_requirements')]
function runtimeRequirements() {
return $this->requirements('runtime');
}
function requirements($phase) {
....
// 1:1 copy of existing system_requirements() minus install-only parts.
}}
}
Partially caught up on this.
> I've been reflecting on this issue quite a bit.
Same. I think I have similar opinions and thoughts as @alexpott, not entirely though.
One thing is I think that system requirements should not be used as an example on how we want to define the API for modules. It's not requirements for the system module, it's requirements for Drupal as a whole, and and as such unique. It definitely has all kinds of valid combinations of phase specific checks. An example for update specific things is checking update hook/function compatibility, like having a schema version recent enough to support update. That's not something we want to run every time you visit the status page. But we do not have to make this fit the API, we can move it out of system_requirements() and put it in an update-specific place that we call explicitly. It might make sense to do this before introducing the replacement for hook_requirements().
I agree that (pre) install time stuff is tricky and annoying to support, but right now I think it does serve a purpose. I don't really agree with #55:
> I think we should say that sub modules don't get to have different PHP version / extension / composer dependency requirements. Because all those should only be defined in the composer.json in the future. That's why it is a sub module :) The one exception to that would be an optional integration with another module.
That one exception is actually quite common, and it's not limited to modules. You can also have a submodule that requires a composer dependency to do something, or a php extension. I just added such a module to the monitoring project. You could argue that it should be a separate project, but I'm not really willing to split development into multiple projects. Per #58, a common approach for that is indeed using suggests, but there are a number of alternatives. I'm almost certain we have an issue that wants to add support for required extensions to .info.yml. We could add support for composer.json instead, there are also open issues about moving at least part of what's currently inside .info.yml into composer.json files. The interesting bit is that drupal.org already now exposes each submodule as a separate composer.json, I'm not sure if it supports merging in an existing composer.json. If not it probably could be implemented, and drupal core definitely could/should check a composer.json file for dependencies. So arguably drupal.org already kind of supports a concept similar to git subtree splitting, except just for composer, which is sufficient?
That would cover most cases why you currently need install requirements. There are likely still going to be special cases though: The redis project even requires one of two extensions OR a composer dependency. You won't be able to declare that in composer.json/info.yml. I don't think it bothers with checking that atm, because it needs settings.php configuration to actually do anything anyway.
There is at least one existing issue about this where I argued that the documentation is pretty weird as this is a form callback and form callbacks don't have return values.
Merged.
Changes make sense, we also discussed this in Slack today. We should not trigger uncaught exceptions for something that can be reproduced through the UI and doesn't really have a workaround through the UI either.
This aiso blocks 🐛 ContentTranslationManager::isEnabled() can load/create the same config entity many times Active although there's more that we might want to do there to make it cleaner and not trigger a warning.
This causes a nasty test fail.
Drupal\Tests\node\Functional\NodeTranslationUITest::testTranslationUI
RuntimeException: Adding non-existent permissions to a role is not allowed. The incorrect permissions are "translate article node".
What happens is this:
1. We uninstall content_translation.
2. that collects all configs that depend on content_translation, including thee user role with the translate permission and the language content settings.
3. This then calls into \Drupal\Core\Config\Entity\ConfigEntityBase::onDependencyRemoval() for the language content settings config entity, it removes the content translation third party setting.
4. Then it goes into Role::onDependencyRemoval()
5. That collects all permissions to figure if they depend on the thing that is being removed.
6. That calls \Drupal\content_translation\ContentTranslationPermissions::contentPermissions
7. That calls \Drupal\content_translation\ContentTranslationManagerInterface::isEnabled() to check if the article bundle is enabled for translation
8. NEW: Because this config entity is now statically cached, it returns the same object as the one that got its third party settings removed in step 3. Which means at this point, article is already not translatable anymore
9. It doesn't return the translate article node permission.
10. \Drupal\user\Entity\Role::onDependencyRemoval() ignores undefined permissions.
11. But then it explodes when trying to save in \Drupal\user\Entity\Role::calculateDependencies() because the permission is undefined.
The whole thing is very fragile.
I see two options:
a) It seems weird to me that Role explodes in case of an undefined permission in calculateDependencies() but ignores it in onDependencyRemoval(). Maybe we should remove them?
b) Clone entities that we're about to change in \Drupal\Core\Config\ConfigManager::getConfigEntitiesToChangeOnDependencyRemoval so that we alter things that others might depend on in their logic.
Rebased and removed an unused variable that is now reported on by phpcs, no other changes.
I'm not saying it's a good idea just that it's possible:
foreach (range(1, 5) as $i) {
$container->register('mymodule.foo.' . $i, Foo::class)
->addTag('tag_foo')
->addArgument($i);
}
But with my follow-up sentence, what I meant is that with plugins, having derivative plugins gives you distinct, reference things, e.g. specific menu blocks that you can place and use multiple times with different configuration. If all you care about is that any of the plugins take care of it, you don't need and you can just loop over your variations in your applies() of a single class.
> Absolute worst case we could 'rename' the per-module skip - ignore the current flag and add a new one.
That was my suggestion, yes. Wouldn't be a huge difference for core. If we need to switch most flags again as an intermediate step we can also go remove it and add it back with a new name once converted. And if there are indeed a handful of contrib that used it then we wouldn't break them.
The in-file-skip attribute is currently quite useful for a handful of modules that have lots of functions like, such as locale. but unless I'm missing something, it's also really just necessary at the moment because we still have some gaps in hook support, such as requirements, info and module_implements_alter. once those 3 and this land, we don't need a partial skip anymore, do we?
Reviewed.
> I agree, but my concern is that, by adding an API to support that use case on the entity class itself, we are encouraging people to take advantage of this pattern even more, which is likely to result in abuse, since it's hard to tell a legitimate API usage (very few IMHO) from an invalid one (most of them IMHO 🙂).
Fair.
> The $displayContext would be passed around as needed and discarded once the render array is returned.
That's the tricky part. There's a surprising amount of places where ->_referringItem is available that would possibly need to be refactored to be able to receive that. FWIW, most places probably _do_ have access to the render array, similar to your view example, so that might be a feasible option.
That was my reason for going with the getOriginal() method in 📌 Define 'original' as property on the entity object Needs work instead of #1480696: Move $entity->original to a separate hook argument → as entities being saved are passed around so many hooks and methods.
I guess my concern is that if we have to solve every single instance of these current cases, deal with BC and so on then we won't be able to do something about 📌 [meta] Deprecate __get/__set() on ContentEntityBase Needs work for many, many years. But it's a fair argument that we don't really gain much in the end if we replace it with a 1:1 API doing the same thing in a different color.
First, I don't think the cache tags system was ever meant to handle thousands of them. Merging is one thing, but it also implies that checking them has to do queries with thousands of placeholders, which generally doesn't seem like a great idea.
At that scale, it might be better to just switch to list cache tags?
That's even more true with cache contexts, there really should only be a handful of those. I was just thinking about when I once accidentally implemented a cache context with high variation. The problem with contexts is that they are sticky, so if you for example add a cache context with the node id as parameter and add that to the breadcrumb block, over time you will have thousands of unique cache contexts there.
That said, I've definitely also seen considerable overhead of merging cacheability but it also tends to be blown up by xhprof/blackfire as that tends to add a lot of overhead to many but fast function calls. For example on the permissions page. So looking into improving that is definitely worth a try.
Yes, I was just thinking about language negotiation stuff while I wrote my comment, some of them actually do have some configuration, but it's configuration that's stored globally in a single place. Can't have differently configured negotiator plugins.
You can kind of can register multiple services with a service provider, instead of the plugin definition you could pass in that logic in the constructor. But container-build time is trickier to manage, many things aren't fully available yet then. But you could also loop over something within the service itself. Coming back to the configuration point, if you don't need to manage differently configured variants, that should be fairly easy.
Does it? Earlier versions of core have a replace statement for this.
I recommend not using an open ended version compatibility. This can't be undone, this version will forever claim to be compatible with any core version.
Yes, even installing the language module with all config doesn't help because it happens too late, *if* the recipe also adds a specific language. It works if not.
So my workaround is to install language with its config first in a separate recipe and then add the specific language.
I think recipes that add specific languages are fairly uncommon (my use case is setting up a demo site), I also argued against making that a recipe with input action in Drupal CMS, as we have a fairly obvious UI for adding languages that is IMHO clearer than having to re-apply a recipe. And adding languages typically kicks of a large and slow batch process to import translations, update config and what not.
But it's still a very ugly workaround.
I still also think that recipes should install non-optional configuration of modules, but then I also think that we might need another mechanism for modules for something like *recommended* configuration. So only things that are required for a module to function would be in config/install while optional things like admin views and so on would be in config/recommended. Which might actually already kind of work if we put it in config/optional.
> For the language switcher stuff, this is a way to say that cache keys should not be set rather than max-age: 0 right?
Yes, exactly. I've wondered about solving this by adding another negative value (-2 or so) to the max age system, but I'm concerned about BC and not sure if there really are other use cases for this outside of block plugins.
Another use case for that are very simple block plugins where I can't imagine that loading a cache from mysql/redis is faster than just rendering it. A core example would be \Drupal\system\Plugin\Block\SystemPoweredByBlock. Especially when it's combined with some dynamic context like the current path. A real world example for that that I've encountered a few times are ad placeholders that are just a single div with some classes or data attributes, they might be very dynamic (based on path, possibly even the user if you for example have a system where paying users don't get ads) but caching them is overkill.
Not saying this should solve this here, just wondering if we want to consider a more generic method. Or we don't, if we can add this then we can also add another specific method, might be better DX.
Closing as duplicate of the referenced issue.
I'm very confused why this is mixing templates and tokens. templates usually implies twig templates, and twig templates don't resolve tokens. Neither seems "default installed setup" relevant, tokens either exist or they don't, it's not a default setup/configuration thing.
You can add OPT_IN_TEST_MAX_PHP: 1 to .gitlab-ci.yml to run that by default here.
I did add https://git.drupalcode.org/issue/paragraphs-3495866/-/pipelines/386551, but it might be tricky to find paragraph specific problems between the messages from modules our tests depend on, specifically search_api, so would probably be useful to fix that first by creating a similar issue there.