About the CR and things being removed without replacement, search for "There is no replacement". tons of deprecations like that, for example node_mark(), which has this CR: https://www.drupal.org/node/3514189 →
Personally, I'd not even bother with a CR and see if someone would prefer to have one. For me, with change records, it's always a question of value vs noise. There's not much useful that we could write except that you'd need to copy paste the code, which seems obvious, and the fact that there are no known usages on that. On the other side is the noise: dozens to hundreds of people will see some kind of notification, like a tweet, toot or whatever. But that's my personal opinion and the documentation page about that is still a bit vague.
I don't know too much about XB yet, but I tried a long time ago to do this with page manager, which turned out to be hard to impossible.
A lazy builder must be able to pass all necessary information as a serialized string.
block.module blocks are 100% standalone and isolated, identified just by their config entity ID, so that's easy. The problem for page manager was context (the block/plugin context system). For example, you could have a view with a term argument, and on the page manager page, you have multiple terms as context that you pass in to the various views blocks.
I think XB does not currently have anything like that and not sure if it ever will. But even then, you don't just have a string/ID, You have the whole configuration set for a given block, which might very well include internal information, maybe you have a block with an API key in the config or something like that. So that can't just be serialized out into the HTML placeholder, might also be quite long. Maybe some kind of key-value lookup, a bit like the configuration for entity autocomplete form elements.
The autoplaceholder config is config. We can change the defaults, but it wouldn't really apply to all existing sites that have customized this. IMHO that makes this very hard to control and those sites will then have negative effects from not having that.
I also think that cache tag bubbling is confusing with that special cache tag. I'm not exactly sure about exact behavior, but it seems unpredictable. As we learned recently, before ✨ Optimize placeholder retrieval from cache Active , placeholdered elements were still included if their element was already render cached, there might be other unexpected side effects.
Also, autoplacholdering doesn't disable caching like max-age 0 does, it would still be cached on its own.
It might not have been clear in the issue where we added it, but I already had this issue in the back of my head to use it here as well, that's why I moved it to the cache namespace and made it more generic. We should extend the docs to mention block plugins as well. In my mind, it's less about supporting CacheOptionalInterface in render arrays (we don't), it's supported specifically for block plugins and makes them not cacheable in the initial definition, but that's an implementation detail of BlockViewBuilder. Other places that support block plugins, such as layout builder, twig or block_field do not currently support per-block caching or placeholdering at all. Which, to reply to #107 as well, is IMHO OK, because it says "cache optional", not "must not be cached". In other words, "not worth caching on its own", which I think fits the use case in access policies.
Started reviewing this, not done yet.
* Yes, #96 is exactly what I was thinking, we lose access to the convenient field item class method, but that's not really internal information, this can be done generically inside the storage and then we we can remove the interface, trait and remove changes to field storage definition classes, which makes this more reliable in case there are custom versions of those.
* the multiple aspect from #96 is complicated, that would require considerable more refactoring. without looking at the xb issue, I'm not convinced that it's a good idea to give field types that much control over this.
* the serialize concerns I think are taken care of I think, that part is and remains in the storage class. might have already been like this when I quickly looked at it when I wrote #58. but I need to have another look at all this.
Should be a merge request
This is a duplicate of 📌 Create a new NodeRepository for the code from Drupal\node\NodeStorage Active , older, but also no work done here, so closing.
The check is in preparation for the new hook in Drupal 11.2 to avoid additional duplication. The changes remove the relevant todo and there is no test coverage for the bug that this seems to fix.
We have fairly extensive duplication tests, if there is a bug then that should be covered by tests.
I quickly tested that this doesn't break ultimate_cron as a module that replaces the cron service class seems to work fine and makes sense looking at the changes, just wanted to make sure. It has its own proxy class that can be removed once it requires 11.2 then.
Removed the unnecessary check for $global_preprocess, it's always an array and check for not already been called first. Also added some more comments, they're a bit repetitive, but it is a bit confusing and maybe that helps understand it.
Note sure if we want to do a change record to call this change out. Again depends on how much we consider the theme registry structure intrernal (which I think we should).
What I just tested manually is that you can move the provided menu link to the navigation user links menu, and then it shows up there. I think this issue predates that this section is a menu, might only be in 11.1 or even 11.2
You only have one of those links though, and we also have sites where we have non-editor users without toolbar/navigation access and then we like to put the link into a frontend menu.
Maybe the module could provide an extra link for that menu if navigation is enabled in hook_menu_links_discovered_alter? that's similar to how navigation defines its own dynamic links.
Still an issue IMHO
I see, a custom exception makes sense so that we can handle this differently for thee return vs notify scenario. I was thinking we could just always thow a redirect exception directly, but that's weird on the notify route.
> Also, maybe we should not always throw the base PaymentGatewayException
Nobody is going to look at it other than commerce, and all it cares about is that it's either a PaymentGatewayExeption (user error, telling the user to retry) or another exception (which tells the user that it's a system error and that they should contact support instead of retrying). IMHO, more specific exception classes are in practice not really relevant.
What is more relevant is whether or not the thrown Exception is PaymentGatewayException or a subclass of that. Should SaferpayException extend from that or not? And should we wrap other client exceptions in PaymentGatewayException. I'm tempted to say yes, because that's the current behavior, and by not doing that, it results in a different message to users. Which might be more correct, but it's maybe also unexpected, untranslated and so on.
Thoughts?
I still have some concerns over the current architecture, see #58, but haven't been able to follow-up on this. I'll try to review this soon.
I replied, setting back to RTBC after answering that question.
We could look into moving template_preprocess into the initial preprocess hook, but with the alter hooks and stuff, I think it's a bit more BC to keep them in there. There are currently also some very weird edge cases with template_preprocess and the base hook key that can result in multiple preprocess callbacks. See the system template preprocess conversion issue.
The reason it's split in and after the loop is because no, template preprocess callbacks are still part of preprocess functions and we need to do run it after those. There is afaik only a single views/contextual integration test that fails if we don't do that, but it's important. That one also failed on my initial attempt in the preprocess OOP issue to split up discovery of global preprocess.
This exactly replicates the logic in the current addFixedPreprocess which this removes, which also inserts them after any template_preprocess function in the list.
What you propose is what we will be able to do once template_preprocess is deprecated and gone, but not before.
FWIW, I still think we should change this, see #12 and also https://berdir.github.io/recipes/#/9/2. But up to core maintainers to make the decision to discuss further or get rid of the todo per MR.
Redis used request time
coding standards failed.
The order change makes sense, back to RTBC.
Do we want to do something about #11? Specifically, do we want to indicate or completely hide this block when placing a new block? We don't have an official way to do that anymore and the referenced issue is actually going to be tricky to implement now with typed classes and constructors, but we could add a a hardcoded workaround.
I don't think that test would fail, because we correctly handle expiration set on items, you need to do a version with TRUE so that invalid items are returned and then expect it to no longer do that. Only then you could actually make it behave differently. See the expire check in \Drupal\redis\Cache\CacheBase::expandEntry.
What I suggested in #21 is that we do this as an integer instead of a boolean flag. NULL would mean the current behavior, also for BC, and any integer, including 0 is the offset added to the provided expiration time. This would only be added to the TTL set in redis and not the expire flag.
IMHO, it makes sense to keep this. I think RSS is seeing a little bit of a push again together with resistance against big social networks and I think it makes sense to support that as an open source project.
The syndicate block is a hardcoded relict from before views was in core and things were hardcoded, that's very different than the generic integration that views has.
griffynh → credited berdir → .
Pipelines have issues, that's not the fault of this issue, unit tests not passing only on 8.4 is _extremely_ unlikely to be a real issue.
I think discovery doesn't have access to the plugin manager. But a property on the annotation class for the min version could work, we could add that.
I created https://www.drupal.org/node/3522776 → and also updated to other two change records to link to this issue as well.
Even if we'd only support composer, there are still things like submodules that have their own dependencies.
I see. Result is basically the same, but it avoids logging it as an error and displayed message is better.
seems awkward to redirect from return to cancel, only for that to then redirect to the actual desired target. Especially since that will actually all back into onCancel() of the plugin.
Merged.
those views are not built with caching in mind, there are custom plugins that aggregate stuff that will not be properly reflected.
Caching is only sensible if it it's viewed a lot more than it's invalidated. That's quite unlikely on these views.
Merged.
Merged, untested.
🐛 Authorized time is not set correctly Active
Merged as part of ✨ Make it compatible with latest Saferpay API Active
Merged.
I'm not sure what you mean with that, cancel works and is covered better than before, for example for PostFinance. But you're welcome to open new issues if you see problems that you want to improve.
Thanks for 1, makes sense, I added one small note on that.
For 2, it's not 100% explicit on the intent, but we do have performance tests that explicitly assert the page cache and dynamic cache page on umami, which is multilingual with language switcher.
We no longer have any changes in those tests because the behavior for them is unchanged in regards to the language switcher block, but earlier merge requests had changes there.
Rebased, I think this is still useful and has a bigger impact than manually adding a few StopProcessing attributes.
> I'm not sure how I feel about making it protected, it is technically api now, not sure why we want to restrict this further.
I think we similar discussions before.
Not every function is automatically an API, reducing API surface isn't a bad thing (for things that aren't useful). Making it protected/internal means it's easier to change and refactor it in the future.
There are no known calls (just one reference for someone doing something similar, it's just a way to share the code between the two hooks. It's exactly like what is done in 📌 Clean up hook implementations in the Taxonomy module Active .
Noted, then I think we either wait on the tests to all be marked as deprecated in 📌 Deprecate migrate source plugins Postponed or for D12 when it's gone.
This is just for runtime deprecations in tests. Doesn't stop anyone from writing a general purpose phpstan/upgrade_status detection for classes in plugin namespaces with annotations and no attributes that would also work on migrate plugins.
I think I like #3 because it's forward compatible. If someone is going to need migrate_drupal in D13, this needs to be done anyway.
Reverted the migrate change, created a follow-up and pointed the @todo that: 📌 Triggering deprecations for migrate plugins using annotations and plugin types not supporting attributes Active
berdir → created an issue.
Something is off with MigrateSource.
It looks like 📌 Move source_module and destination_module from Migrate to Migrate Drupal Needs work has reverted 📌 Convert MigrateSource plugin discovery to attributes Active , all MigrateSource plugins use annotation again?
To be able to proceed with this, I would suggest we open a separate issue to fix this and then add the deprecation for migrate plugins there?
Resolved the threads, updated one reference to this issue to the other one where we deprecate those classes and updated the migrate class to trigger the same deprecation, nice catch.
Found a bit of time to work on this.
Pretty straightforward now, one test .module file less.
Also 🐛 RoutePreloader loads a lot of routes Active , that is a considerable chunk of memory savings there. Need to get back to it and summarize the current state. I think it's fine to not keep this issue as a meta.
Merged.
Seems straightforward and I guess copied from core, but the default is eager, not lazy, so I think it should default to that to not change the behavior.
https://developer.mozilla.org/de/docs/Web/API/HTMLImageElement/loading
Needs to be a merge request.
Merged the other issue.
Merged.
berdir → made their first commit to this issue’s fork.
I updated the issue summary to answer some questions from @dww about min version and D12/13.
I'll try to update the MR tonight (CEST), if someone has time before that, feel free to pick this up before that.
We seem to have multiple overlapping (meta) issues now. The block issue is now essentially 📌 [pp-1] Mark several more modules as converted Active and also does DI, regroups hooks and so on. And there's the template_preprocess meta issue.
I'm unsure what the best way to split these is. The way it's done for block now kind of means we could just inline the actual last step that this aims to do (add the flag to services.yml once ready) into the cleanup meta and close this. I think the performance gains for those last few remaining modules is minor to tiny and it's no longer a priority from a performance perspective.
That said, the cleanup is fairly low prio and I expect those issues to land only slowly as they are hard to review and get committed. IMHO it's more useful to make sure that we've converted all legacy hooks and also preprocess stuff, with the goal of being able to explicitly deprecate legacy hooks and preprocess stuff in 11.3 for D13. For preprocess, we could expand the scope of 📌 [pp-1] Convert Template Preprocess hooks to OOP equivalent Postponed: needs info to not only cover template_preprocess but all preprocess in modules. And I'd probably approach the hooks similar to 📌 Triggering deprecations for plugins using annotations when core plugin type has been converted to attributes Needs work . Figure out how exactly we want to deprecate legacy hooks, that will tell us which bits we missed, and then we can possibly have a single issue to convert those.
griffynh → credited berdir → .
You could try to extend the cacheability and not lower it, but that's risky, because you don't know where exactly that comes from. There might be other tokens that do need a lower max age.
It's the "current-date" that gives the uncacheable metadata, not the formatting. While altering that based on a custom format is possible, it's tricky, because if you do [node:created:custom:s], you'd add a very low cacheablity where it's not needed because the date it depends on is actually stable and won't change unless the node is saved (which is covered by the cache tag).
Did you see https://drupal.slack.com/archives/C1BMUQ9U6/p1744824193034209, this looks very related to that discussion.
Looks good to me, needs a change record, not aware of a contrib implementation that would actually need to implement this, but who knows.
Random idea, but what if we don't bother with session_gc() and instead just call the responsible service directly? Is that an API we can rely on? Then we don't need to worry the problems with session_gc()
Tested on our install profile, by printing out echo strlen(serialize($this->registry[$this->theme->getName()])); in \Drupal\Core\Theme\Registry::setCache().
On HEAD, with the preprocess OOP issue: 171994
With this MR: 158078
That's 92%, so about 8% reduction, and also less code complexity, especially long term. The gain is also bigger the more theme hooks and suggestions you have.
Also comparing against just before 📌 Handle module preprocess functions Active was committed, that gives me 164180. So technically, it is also 4% smaller than HEAD was before that but the invoke map will grow again once we also support OOP in themes.
Pushed a first version of this. Reduces quite a bit of complexity in Registry::build(), $old_cache and addFixedPreprocessFunctions() are just gone.
A little bit of extra complexity in ThemeManager, but that is temporary until support for template_preprocess is removed, then we can just call them between initial and preprocess callbacks.
contextual functional tests are passing with this, might need to adjust some unit or kernel tests.
berdir → made their first commit to this issue’s fork.
Left a suggestion on how to handle that in a way that I think is easier to understand.
Looks good to me. The plugin hook is a bit tricky to categorize as it's a generic hook, could argue that it's either a block or a layout builder hook. But no big deal, just a test module too. Not sure how much we even need to split hooks in test modules, but this one has quite a few.
I'd suggest we keep this postponed on 📌 Convert template preprocess in system.module Active
Still postponed on at 50% of 📌 [pp-1] Convert Template Preprocess hooks to OOP equivalent Postponed: needs info , template_preprocess is actually the main thing here, module preprocess aren't really in those files. I'm aggregating the relevant ones from the meta to check what's left, but keeping at postponed.
Thanks for testing and confirming. Merged, a release will follow soon.
What if we interact directly with the cache tag checksum service only and assert what we want to know with that? Instead of indirectly going through cache entries? That feels convoluted and is IMHO tested elswhere.
Something like this:
// invalidate the tag through general invalidator service.
// assert current value.
assertEquals(1, $checksum_invalidator->getCurrentChecksum([$tag]));
// purge through general invalidator service.
// assert current value
assertEquals(0, $checksum_invalidator->getCurrentChecksum([$tag]));
the database query we can keep.
and we can still do the purge through the cache_tags.invalidator service to make sure the loop there is correct.
(this was a reply to #56, I had a crosspost then with #58, still posting it for the bit on phpstan)
I can do it here, but I meant to deprecate in the other issue, not remove. So that we can land this, especially to get the D12 deprecation into 11.2
We still conditionally call those classes in DefaultPluginManager, and I just verified that phpstan complains about that, so we need to add additional comments o baseline changes for that.
See the referenced-by 📌 Deprecate AnnotatedClassDiscovery Active , @catch opened that recently before I updated the title here, so I thought we could do that there.
Rebased again.
@macsim: I agree, this is postponed on other issues. I'm just keeping the MR fresh as we're using a patch from it and chasing HEAD. I'm unclear how much yours and this issue really overlap, yours got in now and I had a conflict, but I think the changes that are done in this MR are still needed. Would appreciate if you can have a look at that and confirm.
I'm not sure. I think the last paragraph in #17 is IMHO something that should be done here correctly from the start, but as I wrote, we might need to postpone it then on a new child issue of 🐛 Prevent saving config entities when configuration overrides are applied Needs work .
The first part of #17 is more complicated. overrides will not address that. It is IMHO problemic to update roles like that when we are in the middle of a config dependency recalculation and only works accidentally. a proper solution might need a new event or so in the config dependency API?
I think it's OK to re-RTBC this. Had some minor feedback on the HookCollectorPass, and would still prefer to land 📌 [PP-1] Determine how to implement Form Alters with attributes. Active first, this is more important but that would slightly reduce the scope. See #116 for details.
> I did a very careful line by line comparison, The main changes are in HookCollectorPass as expected. I also applied the last scope changes suggested by @larowlan and @berdir, namely making PREPROCESS_INVOKES private and an internal method for returning the array on ThemeRegistry.
FWIW, I'd kind of prefer to just stick to the magic name and avoid exposing it even as internal. 📌 Explore data structures for long term OOP hooks in themes Active *might* just completely remove the concept of the invoke map again. But no big deal either way. We agree it's internal and can discuss further over there.
There was some new feedback about variable names and array structures. I think that was resolved, but also want to mention that this IMHO all internal and partially due to BC layers and we have multiple specific follow-ups to clean this up in the future. Once BC is gone (also for themes, which don't even support this yet, separate topic), we can clean up further and possibly considerably simplify all this.
I'm not sure if we even need to test cache bins at all there. Possibly for the reset() edge case, but yes, then I'd add an invalidate first.
Saw the e-mail notification and came here to say that this should also work fine for protocol-relative paths as we just look for starting with /. but you edited your comment already.
That said, protocol-relative paths are discouraged and should not be used anymore, just always use https:// (or root relative paths), see https://www.designcise.com/web/tutorial/why-protocol-relative-url-are-no....
Properly updated the test assertions now.
Asserting multiple deprecation messages is painful as that's somehow done as a single assertion. Having multiple deprecations asserted together often doesn't work, so I had to add another test method to be able to assert the deprecation message from DefaultPluginManager. I think it's useful to have explicit test coverage as it's generic/dynamic.
I updated the deprecation messages and linked to the correct change record.
I also made a few updates to the original change record to mention that attributes and annotations can be combined to be backwards compatible and also mentioned the error handling stuff a bit, feel free to adjust: https://www.drupal.org/node/3395575/revisions/view/13465802/13973808 → . I think that makes more sense than a new change record even though it's quite old. It's not a change you really need to be aware of unless you run into into problems (and are not yet on Drupal 11.2)
Posted a review.
> -write some Rules in my SQL for purge all CASH each hours.
I'd recommend not purging all your CASH every few hours, that sounds like a very costly thing to do. (Sorry, could not resist).
More serious: If you have a large enough site that you are struggling with the size of the database cache backend and have to flush it so frequently, you should really consider using a backend with a fixed size like redis/memcache. The database backend is a basic implementation for small to medium sites. Also, this is about cache tags only, which should grow far, far less than actual cache backends.
views config dependency removal implementation is still quite bad, but it's almost certain that this module can't do anything about that. fwiw, you probably best configure separate standalone views instead of addition displays on existing views.
Merged.