I think I'd prefer an inline @phpstan-ignore-next line comment over the baseline. That will be easier to adjust once we require 11.3 and remove that call.
Not sure a version check is needed at all, I explicitly requested for that deprecation to be D13, but if we have to touch it anyway thanks to phpstan then we can add it, I prefer compare_version() too. And it avoids the runtime deprecation. As for a comment, no strong feelings, we could slightly adjust the existing one, but it doesn't really matter for this call _why_ it's deprecated. The actual reasons are fairly complex, it's not just because of fibers, it's more that render caching removed much of it's usefulness ages ago as it heavily depends on render cache hit rates how many aliases are needed on a given page and it either loads too much or not enough. But there's no reason to write half a book on that in this module.
drupal/core-recommended does that, not "Drupal". See #3566145-7: Drupal 10.6.1, doctrine/annotations and Simplesamlphp → , you need to remove it to update, this isn't the first time there were conflicts with drupal/core-recommended.
Testing the impact of this might be tricky, both positive and negative.
On it's own, with aggregation off, there's a shift in order of CSS files with this, but it's pretty contained. Diffing it is tricky, because the query strings change on every cache clear, so I put the HTML of node/add/article into a file, cut of the query string for all the claro CSS classes and then compared that before/after this change.
That gave me this diff:
@@ -31,18 +31,18 @@
<link rel="stylesheet" media="all" href="/core/assets/vendor/jquery.ui/themes/base/checkboxradio.css" />
<link rel="stylesheet" media="all" href="/core/assets/vendor/jquery.ui/themes/base/resizable.css" />
<link rel="stylesheet" media="all" href="/core/assets/vendor/jquery.ui/themes/base/button.css" />
- <link rel="stylesheet" media="all" href="/core/misc/components/progress.module.css" />
- <link rel="stylesheet" media="all" href="/core/themes/claro/css/components/ajax-progress.module.css" />
<link rel="stylesheet" media="all" href="/core/modules/system/css/components/align.module.css" />
<link rel="stylesheet" media="all" href="/core/modules/system/css/components/container-inline.module.css" />
<link rel="stylesheet" media="all" href="/core/modules/system/css/components/clearfix.module.css" />
<link rel="stylesheet" media="all" href="/core/modules/system/css/components/hidden.module.css" />
<link rel="stylesheet" media="all" href="/core/modules/system/css/components/js.module.css" />
<link rel="stylesheet" media="all" href="/core/themes/claro/css/components/autocomplete-loading.module.css" />
- <link rel="stylesheet" media="all" href="/core/misc/components/resize.module.css" />
+ <link rel="stylesheet" media="all" href="/core/misc/components/progress.module.css" />
+ <link rel="stylesheet" media="all" href="/core/themes/claro/css/components/ajax-progress.module.css" />
<link rel="stylesheet" media="all" href="/core/modules/ckeditor5/css/ckeditor5.dialog.fix.css" />
<link rel="stylesheet" media="all" href="/core/modules/contextual/css/contextual.module.css" />
<link rel="stylesheet" media="all" href="/core/modules/contextual/css/contextual.toolbar.css" />
+ <link rel="stylesheet" media="all" href="/core/misc/components/resize.module.css" />
<link rel="stylesheet" media="all" href="/core/profiles/demo_umami/css/toolbar-warning.css" />
<link rel="stylesheet" media="all" href="/core/themes/claro/css/components/toolbar.module.css" />
<link rel="stylesheet" media="all" href="/core/themes/claro/css/state/toolbar.menu.css" />
so both progress and resize css files are pushed a bit down, not significantly.
Combined with 📌 Support library-specific CSS aggregates Active , there's no change yet in number of aggregate files. The first is normalize + jquery.ui + drupal.dialog + drupal.autocomplete, so essentially all those that have explicit negative weights. Then a separate one for system.base and then everything else, which interestingly also again has autocomplete and dialog library files, for those without an explicit weight.
Came up with this one-liner to summarize the libraries in a given css group: implode(',', array_unique(array_column($css_groups[0]['items'], 'library'))), which gives me:
0: core/normalize,core/internal.jquery_ui,core/drupal.dialog,core/drupal.autocomplete
1: system/base
2: core/drupal.autocomplete,core/drupal.progress,core/drupal.ajax,core/drupal.dialog,contextual/drupal.contextual-links,contextual/drupal.contextual-toolbar,core/drupal.textarea-resize,demo_umami/toolbar-warning,toolbar/toolbar,toolbar/toolbar.menu,ckeditor5/internal.drupal.ckeditor5,ckeditor5/internal.drupal.ckeditor5.media,ckeditor5/internal.drupal.ckeditor5.mediaAlign,ckeditor5/internal.drupal.ckeditor5.table,shortcut/drupal.shortcut,user/drupal.user.icons,claro/global-styling,system/admin,claro/form-two-columns,claro/claro.jquery.ui,claro/card,filter/drupal.filter,claro/icon-link,media_library/widget
I didn't look into the weights of jquery, dialog and autocomplete yet ,but as long normalize is up there, there's not much to gain with this for claro. It needs a theme that doesn't use normalize and a scenario where you can remove all libraries with explicit weights with this. Which we have.
The current MR is the same as 🐛 Cache is not invalidated when deleting a revision Needs work see my comment there, I think we could close this as a duplicate.
I also partially changed my opinion as we already have a cache tag for revisions that we use invalidate in some cases, so things changed a bit since 2017.
The MR here and in 📌 Introduce revision cache tags and invalidate them on revision save/delete Active which was referenced before is currently essentially the same. I think we should consider to close one issue as a duplicate.
Note that we have now introduced a revision-specific cache tag, specifically "ENTITY_TYPE:ID:revisions" in the entity revision cache issue, see \Drupal\Core\Entity\ContentEntityStorageBase::setPersistentRevisionCache() for example.
What we could look into is overriding \Drupal\Core\Entity\EntityBase::getCacheTagsToInvalidate() in ContentEntityBase and if it's a non-default revision, *additionally* also add that. And then we can invalidate that in deleteRevision()
+1 to rename to key to target or identifier or something like that instead of unique.
Also fine with global. It's not every page, that's correct, the aggregate URL always includes the current theme and language, so it's always varied by this anyway.
And yes, even if there are a handful of variations in the end on the global aggregate target, it's definitely still better than what we have now. The one thing to consider I think is that in most cases, the biggest improvement will be putting the global theme libraries into the the global group. But, how many existing sites and themes are going to bother doing that, 10%? I think it might be worth exploring setting this automatically in a follow-up. Something like if extension-is-theme and library-is-in-global-libraries and aggregate target is NOT set, then set it to global? we could support explicitly setting it to FALSE or NULL to opt out if there really is a use case for that.
@catch. global libraries that would be 'every page' now, and not true, correct?
I think we're still figuring out what the most optimal groups and approaches are here, but I'd say there are a bunch learnings that we can document here.
I can try to draft a change record that covers some of the things we discussed here. there's the bit about not using multiple css groups in themes for best results with this, the notes in #12, some hints for when contrib projects should use this (essentially #12). I'm for example not sure yet if we should also recommend contrib projects to use 'every page' for libraries that they add on every page and when exactly (not when it's controlled by permissions probably, but it's ok if it's depending on a single global setting for a feature that's either enabled or not). Or maybe that's not worth the trouble and risk of incorrectly adding it.
Hi
I'm open to more maintainers, I'm just wondering what exactly you want to improve?
This is compatible with D10 and 11, CI tests are passing.
Note that the real issue isn't the drupal module but the JS library itself. It has been forked, the original version is unmaintained, and both the original and the fork is maintained by a single person, this isn't very sustainable.
Drupal CMS considered and rejected including dropzonejs due to that.
There was a recent post about a possible alternative in one issue that I plan to review whether that's a healthier solution long-term.
Closing as this is patching code that isn't in the module yet, per #7.
Yes, leaving alone normalize.css sounds like a good plan. No test fails/performance changes with this yet. What I'll do with this I think is a diff of the non-aggregated CSS files on umami and claro or so, but I need to find a page that adds at least some of them, because I don't think umami does, at least not on the pages we tested.
Looks good to me, did run the test locally and verified it fails. Tagging since this is required to fix redirect tests on 11.3.
Unsure about the scope and impact here. This obviously overlaps with 📌 Replace custom weights with dependencies in library declarations; introduce "before" and "after" for conditional ordering Needs work , so I'm wondering if we can really split off, doing a quick test.
I think it should work for a library like core/drupal.progress, that's loaded as a dependency of another. However, where it gets tricky is with core/normalize for example. claro/umami/.. load that as a global library, and not a dependency of their own global theming library. That means that normalize.css is not actually loaded first, so it's unclear how that is supposed to work, also with before/after dependencies, since there isn't anything depending on it? We might just need to keep that for edge cases like that?
Thanks, I think this makes much easier to understand what we do there. As mentioned before, I think I can RTBC this, since I haven't touched the implementation changes while you have reviewed the tests I wrote for it.
Not sure about priority here. I think wasn't clear on my 10.x comment back then in slack. This is definitely a new bug, what I experienced in 10.x was something else that is also related to preprocess for theme suggestions, but is different. This is definitely a regression and probably could be seen as major.
I created 📌 Remove explicit weight from CSS files in core.libraries.yml Active and verified that removing the weight from progress.module.css and ajax-progress.module.css eliminates the extra aggregate. And on another page, the same applies to resize.module.css. means they are now after system/base, but that really shouldn't matter?
> The same js would appear in a maximum of two different aggregates, which is a lot less than if we don't specify a unique library at all where there can be a very high number of variations. And most pages tend to have both anyway, then we save an http request for a very tiny file.
Yeah, that was my idea. We get at most 3 variations of those 2 files and save one request when both are used. Not sure if the regression in the test happens just with this change or when you also tried to do the drupal ajax dependencies bit.
> I think you might have misread the drupal.ajax dependencies as being for drupalSettings (because it also specifies some drupalSettings defaults just above) - saying this because that's what I just did checking this before I realised.
Yes, you're right, it's ajax dependencies. Fine if we don't look into this bit further for now, we can revisit this when at least in core, we no longer depend on drupal.ajax. And if someone cares enough, it can probably be optimized for a specific site in a library alter hook.
> I've been doing dump($groups) at the bottom of CssCollectionGrouper::group() fwiw. That might be a place the webprofiler module could override too?
You're right. There's some extra processing in \Drupal\Core\Asset\CssCollectionOptimizerLazy::optimize() but yes, that has no impact on how many groups there are, just how exactly the resulting URL looks (aggregated/processed or not), that doesn't matter for this.
Yes, that's my point. The two URL's must be the same, the CSS URL would need to use the same version string. I guess if the following definition works then that's also OK?
https://fonts.example.com/font-regular.woff2?version=5:
preload: TRUE
early_hint: TRUE
I'm just saying that this is something that you need to keep in mind, it must be the exact same URL, and if you have it in two different places, it's easy to forget to update them if it's not automated.
Some findings/thoughts from more testing:
* drupal.progress, due to it's negative weight on the css file, ends up being its own aggregate in front of system.base when loaded
* putting everything in our global theme library into the theme group reduced the total number of aggregates from 8 to 4. So I think a result of this is that it's definitely recommended to avoid multiple groups when using this feature and that's worth documenting.
* investigating and optimizing this isn't trivial. I ended up setting a breakpoint in \Drupal\Core\Asset\CssCollectionOptimizerLazy::optimize. it unsets the items, so a wrapper of that wouldn't be able to access that information when returned. This could be a useful addition in webprofiler or so, to report on how many aggregates are on the page, what's in them and how big they are. maybe the css_assets loop could use a protected method just to make it easier to allow insight into this before the cleanup in a subclass?
* On the JS side, I for example get jquery.min.js in a separate library, that's already the case. I'm also getting one for for once.min.js (again, already the case due to preprocess false), and then also the drupal aggregate with drupal settings. wondering if it's worth merging once.min.js and the drupal aggregate instead of the preprocess false now?
* will need more time to understand the JS side, see quite a few asset sets that are separate, such as loadjs, tabbable and progress.js both being in their own separate thing. They're all dependencies of drupalSettings, which is in the drupal group, maybe we need to add them to the drupal unique key explicitly to put them together?
Not caught up, just one note on fonts.
We added something like this (just preload, not early hints) to https://drupal.org/project/bs_base, by adding preload fonts to theme.info.yml. One tedious bit was cache busting, we for example have an icon font that updates pretty frequently and because this isn't the canonical font definition (that's in CSS), it's very easy for the actual font-face in CSS and this preload info to get out of sync, resulting in loading two different files. That doesn't just defeat the whole purpose, it makes it worse than not having it at all.
I'm not sure what a good solution for this is to be honest, in our case we then added support for updating the .info.yml definition into the icon font build toolchain. I think if core respects the library version and adds that to the URL, similar to CSS/JS assets then that's probably OK and we could update our toolchain to maintain the library version for us?
I think it's OK to get it in like this without test coverage, steps to reproduce are not entirely clear, but this stops the bleeding. The whole theme registry structure and build process is due for a refactor once all the BC layers are gone.
@nicxvan I think with the new RTBC rules, it's perfectly valid if I review your fix and you review the test coverage. If you check my feedback and remove the extra test coverage and review my test then I'm OK with setting this to RTBC afterwards.
Most of those tests add no value, I'm going to assume they were generated. We don't need to test the config API. A single test to save the UI would be sufficient. There also don't seem to be tests for the harder and more useful part of the actual logic where the config is used.
Configurable loglevel also seems overkill and if done, should use the log() method with level argument.
I'm open to just removing this without any setting, since there's no value in this information. That would also allow to simplify the code, we don't know the know the count, I'm also not sure why this does an update plus a looping select and a delete.
Tests contain explicit calls to the install hook, that needs to be updated.
There's quite a bit of discussion in 📌 Drupal 11 compatibility Active on the migration to samlauth, in #27 someone created a migration module, I didn't try it.
(crossposted with your self-answer, but still posting it now that I wrote it...)
That's not entirely clear yet, I think there's no specific plan, also depends on what you understand by deprecating.
procedural hooks are deprecated, for module and themes and support for them will be removed in D13. There are no phpunit or phpstan deprecations/rules yet for that however.
@nicxvan and I have slightly different opinions on the approach, I prefer to start deprecating early and more explicitly (we currently only trigger deprecations. But I've also seen what happens in larger contrib right now with the plugin annotations we added, any test gets you a massive wall of deprecation messages which can be tedious when you're trying to for example fix tests. Hooks would likely be worse and depending on where you we do it, could happen frequently enough that it might have a measurable performance on real sites.
It's possible that we'll skip deprecating that entirely and just directly deprecate .module files as I said in #73. For that, we first need to get rid of .module files in core, we're actively working on that here: 📌 [meta] eliminate core .module files Active .
Personally, I'd vote to close this as won't fix. Per #65, this *will* result in fatal errors in contrib, I don't see how we could prepare or deprecate for that and our efforts are better put into getting rid of .module files
drupal/core-recommended is known to cause conflicts with more complex dependencies. It locks down specific minor versions that other projects conflict with. Removing that allows to install the most recent version of simplesamlphp.
There isn't much that this project can do about this.
simplesamlphp has complex dependency chains that frequently conflict with core, symfony, guzzle and others. As mentioned in the D11 issue, I strongly recommend to review if https://www.drupal.org/project/samlauth → is sufficient for your requirements and switch. We did, and so did many others.
Fork worked now for me, gitlab was overloaded a bit in the last 24h due to bulk MR updates in core.
Thanks for helping to finalize this.
I've merge it. Since test coverage is fairly limited, I'm leaving it in dev for a bit, it should be easy to install and test now, if I don't get any reports about bugs in D11 I'll create a release in a bit.
That's fine, the reason I brought it up is that I'm a bit concerned about the RTBC queue. It's long and these .module cleanup issues are adding quite a bit to that. The main bottleneck that we need to optimize for I think is the RTBC queue and less our own time.
But yes, that one needs a product or framework maintainer decision I guess and that's essentially the same people.
I think 🐛 Accessing a page that doesn't exist causes a fatal error in EntityRouteHelper::getContentEntityFromRoute() Active is another duplicate of this, need to check.
There's also an issue that discusses removing this functionality completely, although I don't fully agree with that. Might want to make a decision on that first before moving it to avoid duplicate work: 📌 Copy block configuration from admin theme when enabling an admin theme Active ?
can we remove hook_install() completely which also doesn't check this as I think hook_modules_installed() is also called for itself?
Are you on D10? Per recent comments, this should not happen on D11.2+, nor should it happen when using the replicate module.
The MR is definitely not correct and should not be used. Feel free to create a new MR with the proposal from #29/#30 as a workaround for sites stuck on older core versions.
I'm not certain that we want to support this. Not really the callback thing, but defining preprocess functions in hook_theme() at all and promoting that. I'm not certain, but if you look at \Drupal\Core\Theme\Registry::processExtension(), there's a check there that skips at least some preprocess hooks discovery if it's already defined, by doing this, you might prevent other extensions from preprocessing your template.
The other reason I'd like to discourage this is what we currently need tricks to make the preprocess functions work with OOP hooks, with the invoke map, and it's still called functions. Once the BC layer is gone in D13, I'd like to revisit the internal structure and possibly adopt an easier structure with less overhead and it would then likely only support hooks.
Calling other initial preprocess hooks from an initial hook is done in a few places in core as well, such as \Drupal\Core\Theme\ThemePreprocess::preprocessMaintenancePage.
That said, I don't see a concern with performance. CallableResolver is pretty fast. If it's already a callable it returns immediately and if not, it's a string split + service lookup. Which can be slow with dependencies, but the overhead of CallableResolver was measurable when I profiled it IIRC. Keep in mind that all hooks go through that now.
No, it just means that it won't be backported anymore to that and that MR is no longer relevant.
#10 looks like you're missing the doctrine/annotations library and something depends on that. See https://www.drupal.org/node/3551049 → and try to require that explicitly. This library is unsupported and core had to fork and switch away from it. This should help in the short term, but you'll need to figure who uses that without depending on it.
#11: I don't think that's the same or even a similar problem. It mentions the same class, but the error is different. No idea why you get that. you may be using patches or somehow have multiple versions of token or some other super weird scenario. Token 8.x-1.17 is tested on D10 and should work fine there. You might get a different error if you are using the latest version. Make sure you only have one version of token, no patches, test on latest version, visit update.php and so on.
Started testing this on our project.
Adding "unique_css_aggregate: 'every_page'" to our global CSS library cut the total stylesheetbytes size to 40% of the previous sum on HEAD when visiting 5 different pages in a row (where I assume all have some unique CSS being loaded).
But, the CSS file count goes from 3 to 8 just by that single change. What's weird is that I end up with 2 completely empty CSS file (the only thing in there is the license comment). We generate child themes from a base theme for our projects which a pretty extensive base structure and it's currently all in a single global library, that library includes 22 CSS files split into base/component/layout/theme CSS groups. Specifically in this demo theme, the CSS files in the theme group, one for media all, one for media print are empty. Already on HEAD this somehow results in an empty print CSS that's also actually being loaded. I think something is off here unrelated to this and possibly a bug on our side, I don't think that should be empty.
But unrelated from that, if I understand this correctly, this splits the single library into different files due to having multiple CSS groups? Is that the expected behavior? Is that because it optimizes for a scenario where multiple libraries have the same unique css aggregate string and it needs to split in case they have mixed groups? Is using multiple groups in a single library discouraged/a bad idea or is this something that should be optimized in this implementation?
I'm always wondering whether something like this should be an addition to an existing test method or a new one since functional tests are fairly heavy. But I think it's OK like this.
This test coverage will allow us to remove the is page code in content _moderation without risk of regressions.
Thanks for the updates. This should be close. Glancing through phpstan, This is something that should be fixed:
Call to deprecated function watchdog_exception() in SimplesamlphpDrupalAuth
Replied again, see #2739290: UTC+12 is broken on date only fields →
This has conflicts, D12 upgrade changes shouldn't go in this.
Some of the non-injected dependencies are kind of like that on purpose because it's only the BC layer, we'd have to remove that again when that's gone.
Thanks for the fix. Pretty sure I've seen this too and was wondering where it came from.
Added to merge train.
Merging. Might want to check how core handles this and if something there should be adjusted as well.
promote and summary changes caused the test fails.
Not sure I get the full picture yet, but does performance really matter? This is very unlikely to happen much in practice, maybe if you manage to create an alias with uppercase characters and then fix it.
If you have redirect, it will then redirect away, and I wouldn't expect that getPathByAlias() is called that often for the same alias.
Based on debugging this a bit, the initial lookup is even part of the router cache, it's then only called for parents and so on for the breadcrumb and they're all unique calls from what I see with my example on umami.
leaving at needs review for now, not sure if we want a more explicit test for this to assert what's used for the return value, could copy testGetPathByAliasMatch() and simulate that. Probably don't want to test the static cache as we know that won't work.
It is worth nothing that the core functionality and this module have a few fairly important differences, that confused us for quite some time when testing and comparing.
The most important one is that core replaces the current history state, while this module pushes new states. What this means in practice is that with core, you can reload the current filters, you can bookmark the current set of filters, you can click on a linked element and then use browser back. You can however NOT use browser back to go back to a previous filter. I'm not sure what's better.
Also, the core functionality is server side, it determines the URL and filters there, whereas this module I think mostly does it in JS. Both kind of conflict with 🐛 Table clicksort is lost when using views exposed filter Needs work .
This module is also opt-in per view, while core is not configurable, it just always does it for all views.
We decided to remove views_ajax_history in or projects and stick to the replaceState approach used by core. Both together is pretty weird. If someone would want to use this in 11.3+, I think an event subscriber to remove the core command would be necessary, as documented hiere: ✨ Change URL in browser with page parameter Needs review
Yeah, that's exactly the case we discussed. Is it really that much of an issue if this is still called? I guess it triggers the deprecation then?
Not sure about extending the attribute, I'd probably prefer a shared interface if we were to do that. but that's less likely to get backported to 11.3, not sure.
I meant to link to #1367354: The list of theme hook suggestions for "field" is incorrectly ordered and has namespacing collisions → I think.
Your error is from webform_node, and not this project.
Also, instead of just a note, I'd prefer making this explicit and require drupal/core ^10 || ^11.3. Will be easier to resolve then for composer as well I think. I'd remove D9 as we no longer test it and have no idea if that still works or not.
This can't be merged with tests failing. Gitlab CI now switched to 11.3 as default version, so testing is unblocked, we just need to disable previous minor now. Agreed that the 2.5.0 constraint isn't needed. It was dev before, which was needed, because they push their releases only inconsistently to packagist, you can see only rc3 and 5 on https://packagist.org/packages/simplesamlphp/simplesamlphp.
Run updates, clear caches. If that doesn't work, Enable verbose error reporting (through settings.php), and post the error.
If you still get this error after downgrading token, you did not actually downgrade it.
I'll have to wrap my head around the implications and changes to this.
Umami has quite a few distinct libraries. The frontpage now for example loads recipe-collections, which contains just 4 rules. that ends up in a separate aggregate now because it's the only extra library. Is it really worth splitting up tiny definitions like this? Do you know of any guides docs around tradeoff between loading unused CSS and extra files/requests?
system/base is also pretty small, and it's loaded by system modules. If I'd want to optimize this, is there any benefit in putting those every-request libraries files into a shared aggregate? Does it even matter with HTTP/2 and stuff?
Will try it out on my project.
The setting was exposed elsewhere, note that support for large jobs might break depending on the size of the items and the translation provider.
This replaces the existing logic, which is possibly fine but doesn't need to run both. There's likely some test coverage that fails because we do test non-supported languages.
This needs to be a merge requests for tests to run.
This contains unrelated test cleanup changes, including the addition of a trait, unclear where that comes from?
Also needs a rebase on composer.json
This method is not on the interface, it also doesn't deal with things like nested elements from embedded entities. To handle this properly, we need to add support to attach upload validators to the data structure.
Also adding to the merge train, another conflict on the webform change ;)
Better to do fixes like the webform change in separate issues, conflicted now. Rebased through the UI and adding to the merge train.
Thanks, merged this in the issue where I fixed the current test fails.
All the actual fixes already happened elsewhere, but was a clean automated rebase and updates some docs the other issue didn't, so merging this as well. Merging.
This includes some functional changes to the preview title, due to 11.3 core changes that no longer show the node title in the default view mode.
Yes, this is a duplicate of that issue, as you can see by the caller.
Maybe shouldn't have deleted it, but it's already fixed in smart trim.
Could you open a MR(s) on the active branch(es) so it's easier to review and discuss the code?
One concern I have is a that a considerable amount of ->entity property calls are going to use an already loaded entity. Just like getTarget() can't figure out if the call came through __get(), this currently can't know if the entity is already loaded or not. I'm not sure what the overhead of this is and it might not be measurable, but it's there. It's only when there is a fiber, but a surprising amount of logic runs within a fiber because rendering always uses one.
In ERR, I recently added the ability to check whether the target is already loaded, see \Drupal\entity_reference_revisions\Plugin\DataType\EntityReferenceRevisions::isTargetLoaded, \Drupal\entity_reference_revisions\Plugin\Field\FieldType\EntityReferenceRevisionsItem::isEntityLoaded, this is used to check if an entity is already set, which fixes some edge cases of replacing changed entities, see 📌 referencedEntities: Use loadMultipleRevisions instead of loadRevision Needs work , core has an issue for this as well: 🐛 EntityReferenceFieldItemList::referencedEntities might not return up-to-date entity objects Needs work .
Long story short, if we were to add the same capability to regular ER fields to fix that issue, we could optimize this to only use a fiber if we're going to load an entity. That might not happen before D12 anyway.
Tests are currently failing, but that's not the fault of this issue. Merged.
Posted a review of how I think we can simplify this a bit.
Not sure what to think of this.
I mean, avoiding that loop is why the field map exists. Aren't we risking to improve cold cache performance at the cost of warm caches and other scenarios? Looking through getBundles() calls, it's mostly, views data, field UI and update/delete hooks around fields.
Should we then consider to deprecate the field map entirely? Maybe solve getBundles() differently? If it's just concerned about configurable fields, maybe we could add lookup keys to field_config config entities and do direct entity queries? I suppose that would be a lot of lookups as we're doing it per field and this is then statically cached.
Not sure yet, just thinking out loud.
I think this is good enough for now. Maybe this prevents bulk loading in a few cases when it would be safe to do so as we don't know if that method is called within a magic __get() or not, but that's better than the pretty common fails on contrib tests and real projects that we're seeing.
I've seen failing tests in the core views as fibers issue (which would likely make this far more common even if we fix the specific scenario by not using magic there), in paragraphs, webform, a groups module and more. It's very confusing when you run into it and while the workarounds are usually easy, understand what you have to change is not.
I think that's more than enough reason to make this a major issue and committing a somewhat ugly workaround like this.