Interesting ideas, but as always, things get a bit more complicated with contrib, with the main challenge being release management/timing where an event invoked by core might be not be the right solution.
A practical example is when we deprecated and replaced the node_type condition plugin with the generic entity bundle plugin derivates. Core provided an upgrade path for block configs. pathauto also uses condition plugins to control which patterns apply to which entity. I did write an update function there, but there were still a bunch of people that did run into problems because they either had old default config in install profiles/modules, might have updated pathauto before core, or reimported fixed config after running updates (a surprisingly common issue).
A solution where a block plugin can trigger an update for all possibly afffected places is IMHO extremely hard, because it would require that people update all contrib and custom code at the same time as core introduces that new update, so those implementations actually get triggered. I think that's not realistic and more complex solutions like discovering those conversion handlers/plugins doesn't seem easier than each implementation writing its own update function. Maybe it could be useful to have some apis on plugins to deal with the conversion, but that won't handle a case when a whole plugin gets deprecated, I guess.
Also, what navigation does with block plugins isn't even config entities, just a single simple config file, so generic config entity based approaches wouldn't work there (fwiw, I think what navigation does is wrong, you shouldn't manage block config in simple config, but that's a different topic)
@brockfanning: So people are in fact doing what we feared they would, "interesting". Note that this improves it quite a bit, but it will still cause cache variations for every query string variation, which could be an issue with random things like utm/social media identifiers like fclid from facebook. And this will bubble up into the dynamic page cache. How much of an issue that is for you I can't say.
The neat part about how it works for regular blocks is that it's combined with the also new createPlaceholder feature, see https://www.drupal.org/node/3512518 → . What this means is that it just adds a placeholder and then actual language switcher links will then be rendered later and the wrapping element (node or dynamic page cache element) doesn't need to vary by those things. I'd recommend creating a twig tweaks issue for this, it might be possible to support this there as well and then placeholder the block there as well. No idea how feasible it is as it obviously doesn't operate on render arrays but within twig rendering.
Checking a single config involves dynamically resolved types in many different config files (e.g. something like views or entity view displays). I think this is not practical.
That was a real fail, fixed now.
The main question I have is regarding MailThemeNegotiator::applies(), which requires the choice of theme to depend only on MailTemplateID. This doesn't "unambiguously identifies the email to be sent" as the IS states but identifies a category (the email also varies with the params etc). I can imagine scenarios where we would require more flexibility: e.g. the site has multiple brands with different main themes. We might check the user who will receive the email, see which brand they registered with, and choose the matching theme.
I think it's perfectly fine if applies() and the actual logic considers global context like current user/domain/configuration, you can just inject that in your service. I don't expect we'd do any caching or so of this logic, so we don't need to be aware of all the things that might influence the decision, we just run through this on every e-mail.
On BC, I think it's fine if it applies to the current mailer if core doesn't come with en enforced default. Either nothing for now, or a configurable setting which could be a 1:1 copy of what mailsystem does, with current/default/specific theme list.
That's only correct if you have autocomplete enabled, otherwise they might be authorized only and that's the expected state.
I'm unsure how you end up with multiple payments, they are only created if at least in authorized state and there shouldn't be a need to do another then?
We just discussed kind of the opposite, that we should drop the return FALSE and return the payment we have, possibly depending on the state and autocomplete setting. That should in my opinion remove the need for those long sleep() calls on notify, because the loadForUpdate() should ensure that it will only have to wait as long as necessary and it's not a big deal then if notify goes first.
#6 was wrong, we were just on an older commerce version, 🐛 NeedsRedirectException not handled by PaymentCheckoutController Fixed added support for that.
Keeping this at needs work, I'll add that back, we are testing this patch now in a project and it already helps a lot, but we're also identifying cases that need improvement, such as error handling on the notify callback. a lot of errors happen there due to abandoned transaction, no need to log these as exceptions, especially not uncaught php exception.
I tested this a bit. The needs redirect exception doesn't work because it's caught, instead you get a message that a system error happened. I instead extended PaymentGatewayException and dropped the custom exception check. I also pass the whole array to it so we can log that.
Aren't the added types a bc break considering that the old classes are automatically aliased to the new ones?
Another review. FWIW, you've convinced me to default to not throwing an exception in D13 and going with that MR.
There are been some reports about things failing with the fallback autoloader, such as this slack thread: https://drupal.slack.com/archives/C1BMUQ9U6/p1748549559293769.
It can apparently happen that the missing trait gets autoloaded as the fallback, but is then actually needed later and things fail when the trait doesn't provide methods that classes require for their interfaces.
We agreed to reopen this.
At this point with the upgrade path worries mostly out of the way, I'd say we should just make layout_builder depend on block_content and add an update function that enables it if that's not yet the case.
I'm a bit unclear what the specific issue is now.
I worked with @arla in 2016 when we did encounter this, but things have changed since then. We did deprecate and remove Drupal::l() and replace it with a Link::toRenderable(), so you should be able to alter this in additional preprocess functions as it is an array and not just a string now.
So it is possible to *alter* file_link template and its link structure and attributes. The thing I see is that it might be difficult to get enough context, for some cases you might want to alter only specific fields or so, then you'd want to alter the render array before this bit is rendered.
Additionally to the title, the issue summary also needs to be updated with the proposed solution and use cases why it's useful.
Is this is about experience builder or navigation?
There definitely should be an icon/integration with the new navigation UI similar to Gin.
But I'm not so sure about experience builder. Coffee is about navigating away from the side, is that something you want to do while editing?
I think modules being installed or not is by far the most common use case for this and I'd rather focus on that. callbacks imply dynamic logic like something based on settings, but then you have to rebuild the container if that changes. Seems more complex to support and define. Could also design the attribute in a way that allows to extend it later?
#[ConditionalRegister(moduleExists: 'config_translation')]
And then we could add a callback parameter if there is a valid use case for that later.
ContentEntityBase::set() calls get(). We probably either need to do the same with set() and add an optional $throws_exception so it can be passed to get(), or we have set() call get() explicitly with $throws_exception as TRUE, so exceptions will always be thrown for invalid fields
I think set() shouldn't silently ignore what you pass to it, so I'd suggest we explicitly tell it to throw an exception.
get() is documented in FieldableEntityInterface to throw an InvalidArgumentException on invalid field names, but in ContentEntityBase::getTranslatedField, which is called by get(), it's also possible to throw an exception when "The entity object refers to a removed translation". My thought then is that not all \InvalidArgumentExceptions should be suppressed, but only ones with message "Field $name is unknown", but this seems brittle
We could introduce a new exception for unknown fields then we can deal with that specifically. That should be backwards compatible if it extends the existing exception.
^11.0 is the same as ^11, So would ~11.0. *Only* ~11.0.0 would restrict it to <11. See https://getcomposer.org/doc/articles/versions.md#caret-version-range-
Just like ^10.2 also also 10.3 and so on.
#3 is just a revert of the change done by the other issue, which IMHO implements the correct thing and is consistent with how translations through the UI also happen.
Will need to be investigated why this happens with site studio, reverting this change is not an option at this point.
As mentioned a few times, but a bit hidden in comments, if you are blocked by this, consider switching to https://www.drupal.org/project/samlauth → .
It's a way less complex setup that has far fewer dependencies and no conflicts with Symfony. simplesamlphp_auth will always be a struggle to maintain. You can even run the two in parallel, you just set up the new samlauth and switch the login link, adjust your hooks to their events. Optionally, you might need to adjust the external login auth map table with a query, in our case we could just rely on the username/email fallback.
We switched, I no longer use this project and will only provide minimal or no maintenance for this project going forward.
> Redis ≥ 8.0 is available under the open source AGPLv3 license too, not just the non-open ones
The problem is that hosting platforms such as platform.sh and also general cloud platforms like AWS and so on can no longer provide redis as a service.
That said, I don't really get what #5 is trying to say. valkey is compatible with phpredis and so on, there is no indication that this module wouldn't be compatible with valkey out of the box. If you see any indication that this isn't the case, then open a bug report with specifics and extend our gitlab CI test coverage to test against valkey. Otherwise, just test it and switch.
benjifisher → credited berdir → .
gábor hojtsy → credited berdir → .
smustgrave → credited berdir → .
As you said, this is IMHO a core bug, especially in this scenario because it's a bug in the BC hook discovery. even if you would define a hook starting with a number, it should cast it to a string.
berdir → created an issue.
Fair enough on a separate module for pathauto, do you plan to maintain that in this project or a separate? I already mentioned, but 💬 Get a token of a node's parent group to create a pathauto pattern Needs review is IMHO a really useful issue for anyone using group specific aliases. The second thing is that we implemented a custom hook that prevents an alias if content isn't in a group yet. Which they initially on the first save always are:
/**
* Implements hook_pathauto_alias_alter().
*/
function YOURMODULE_pathauto_alias_alter(&$alias, array &$context) {
if (isset($context['data']['node'])) {
// Prevent that the initial save of a node in a group creates top-level
// alias without a group prefix.
if (strpos($context["pattern"]->getPattern(), ':group:') !== FALSE && !GroupRelationship::loadByEntity($context['data']['node'])) {
$alias = '';
}
}
}
This might be a useful feature in that integration module too, but it will likely need to be configurable per pattern instead of the token check we use. I also removed one condition we have that then overrides that logic again for one specific case.
This doesn't and never applied to D8+ because there this is a plugin class and already separated.
One of them is 🐛 DivisionByZeroError - Problem to access Status Page on PHP 8.2 Active that you rerolled, another is ✨ Azure/AWS: Take into account CONFIG command may not work Needs review . They all change things in the ReportsController.
Related to the config sync issue but definitely not a duplicate.
That said, I don't really agree with the issue.
Themes *can* provide default blocks and then the default blocks aren't copied. claro does that. If that wasn't done, then copying the default blocks seems like a reasonable fallback to me.
The pipeline fails aren't related to this issue. This is RTBC.
To add, if you add the flag in your services.yml then this won't do anything because we will skip the BC scan completely for that module, so that's even better. But it will take years until the majority of contrib will be converted like that and this optimization reduces the current maybe-legacy-hook list by about 20% in my example, with IMHO zero drawbacks.
> That would fix the concern raised in #4, but it would still analyze all functions starting with the module name and "guess" that it could be a hook and add it to the container, no?
Only for the BC layer for legacy hook functions. We can't avoid this. We don't know what's a hook and what isn't. But this will completely go away in D13 (together with support for .module files)
We also have an attribute already to stop scanning in a certain file, so we can manually optimize this. But it's tedious and confusing to understand.
The purpose of this issue is to avoid adding functions where we *know for certain* that they are not hooks or to be more specific, not hooks that are supported by the new system: update functions, hook_install and so on. It's an automatic optimization, mostly for stuff that's in .install files. It's a very narrow scope and the goal is not to change how the BC system works.
Feel free to reopen if someone disagrees, but with pluggable access policies and so on, this is now no longer needed.
@danielveza: This was a re-RTBC after the MR needed a reroll, that's perfectly fine if there weren't any complicated resolutions. See also recent adjustments to the docs on how to set RTBC: https://www.drupal.org/node/3156237/revisions/view/13965063/13985848 → , sometimes we are/were a bit too strict with our rules around that.
No, path auto does not use state
Added STR, I don't remember 100% if it already happens when you visit the form display page or if you need to click the gear icon of a given field, but one or the other should reproduce the error.
this does a strip tags, that seems wrong and the escape then is pointless?
if the source config has some HTML, you want to see that as the translation needs the same.
It should just escape and should then also assert the escaped markup
Code changes look fine. I guess since this does not fix an actual bug, it could be reclassified as a task, but doesn't matter too much.
I don't really understand your feedback or the status you set.
We now use the proposed MR in production, it works well for us and I think is a far cleaner solution than throwing an exception as part of saving the user and having to deal with edge cases around that. Exceptions IMHO shouldn't be used for code flow/logic that isn't actually an "exception".
So, imho, needs review is the status I'd use here?
On the sync, in my case, I guess the LOGIN event would work as the I have the information I need available on the account as fields and roles, but in general, that event doesn't have access to the SAML attributes.
The deprecation is there. The only thing is whether we want to document on the interface that this will get a type in D12.
I'm pretty certain that nodes with invalid bundles also cause a fatal error when viewing due to template_preprocess_node().
But yes, I'm not surprised that a lot of tests are ignoring that at the moment. There are also some weird things like menu_link_content which defines a bundle but does not actually have one.
Can see both sides, that it's more relevant for media, but also it being minor meaning that it's not so important to push a media specific solution through.
The workarounds in the test do seem a bit strange. They are inconsistent, one uses $this->container, the other does not. also sounds like both cases actually do define them, just the entity bundle cache is still. Which is either a problem with mix of web requests and test code, or there's a deeper issue with a static cache not being properly reset, or an issue with multiple containers and DI (that's why we should IMHO discourage and even deprecate $this->container)
Testing this as we finally have a use case for this.
The chunked upload itself seems to work fine, however, then nothing happens.
Debugging a bit, I can see that despite the comment saying otherwise, dropzoneInstance.on('success' is in fact called with a response, on the last part, *after* Drupal.dropzonejs.chunksUploaded, and it has result filename with the last part. This overrides file.processedName, this is then submitted and obviously the server then doesn't find that anymore.
This is with dropzone 6.x beta2 so there might have been a change with that.
Commenting out that line as a quickfix solves that and the media can then be edited. But it needs a better check, maybe by checking if file.processedName is already set and then assuming that it was set by the chunk response so that it's compatible with different versions?
Also, if you have problems with max file size, then you might be on an old core version, #2867336: File size validator should only respect the explicitly configured maximum file size → should have resolved that.
I'm not sure why this is implemented as a media specific solution, if anything it should probably be done on content entity storage?
Fixed a few more things and adjusted tests for removed monitoring_install()
You weren't far off, had to debug through it as well. kernel tests are hard, there are always hidden subtle things that do not match a default setup.
In this case, some issues were:
* there already were some blocks that were created as claro has default blocks so I emptied that, a test theme might be better for this.
* And then the test passed but for the wrong reason, because no default theme was set in the kernel test, that doesn't happen automatically, so had to that.
added a second test where we expect the initialize, refactored things to initialize shared things in setup, also use invoke() instead of calling the function as this will move to OOP soon.
Ah, the fatal error was actually be the update status sensor and not requirements, but requirements didn't see the new requirements anymore.
Extended discovery to support both and updated the sensor so that runtime sensors are also supported, including multiple implementations.
Absolutely. Task: Convert the first chunk in the patch to a merge request.
Still useful, but per #34, we only need the docblock change.
Updated the second place. I think it would be nice to get this in before 11.2 and avoid fatal errors. I can fix entity_browser but others might be affected too.
Created 🐛 Fatal error when passing NULL to Renderer::render() Active
Proposed a deprecation.
The constructor changes conflict with recent fixes for D11.2 compatibility. And yes, not happy with doing raw queries like that, not due to performance, but it's bad abstraction. Fine as a workaround patch, but will likely not commit this. Try to help with getting the core issue in instead.
Merged.
Considered autowire, that works, but needs two annotations and still have to deal with future constructor updates.
So I switched to setter injection, then I should be mostly future proof.
PSA: This broken menu_trail_by_path due to the constructor changes in a way that confused me a bit.
What that module does is alter the service like this:
$definition = $container->getDefinition('menu.active_trail');
$definition->setClass('Drupal\menu_trail_by_path\MenuTrailByPathActiveTrail');
$definition->addArgument(new Reference('path.validator'));
$definition->addArgument(new Reference('router.request_context'));
$definition->addArgument(new Reference('language_manager'));
$definition->addArgument(new Reference('config.factory'));
So it gets the new argument added here but then its constructor doesn't expect that.
Don't see how this could do anything different, turns out that's a bad pattern for extending a constructor. Will switch to setters or autowire in 🐛 MenuActiveTrail::__construct() changes in Drupal 11.2 conflict with service alteration Active
berdir → created an issue.
The array type added here cases a fatal error for us, caused by template_preprocess_node():
$variables['date'] = \Drupal::service('renderer')->render($variables['elements']['created']);
created is null, and while it shouldn't call it then, this seems like a BC break that shouldn't be done like this or then the render () method should guard against that?
Follow-up to remove that?
Per https://www.drupal.org/project/select2/releases/2.0.0 → , you need to use the development version of select2 which is compatible.
I still think we should at least move the ->get() call out of the constructor and preferably remove the config entirely and just use settings. Anyone who needs syslog should be capable of customizing it through $settings. A bit tricky with BC of course.
That said, the backtrace in #88 doesn't seem related to config, I have no idea what the router has to do with syslog, maybe there are Url objects involved.
FWIW, you can configure a field and disable it there. I don't think there's anything that would make sense to translate? You can't translate a company or person name or address?
> 2 . The maximal allowed value is 1 day. Why is that? Drupal 8 has got tag-based cache system which is supposed to cache never-changing content forever.
The internal page cache is forever, this setting does not affect this.
You can use tags to invalidate content through a supported CDN such as Fastly or Cloudflare (with an expensive plan). But you can _not_ invalidate caches stored directly in browsers, which is what this setting does as well. That's why for example Fastly supports its own cache control key which core doesn't set where you set a low cache max age on the regular header and a longer one just for Fastly.
In short, it quickly gets more complicated and it's pretty dangerous to set this value higher and you should only do that if you have the appropriate infrastructure.
Having to set a value in seconds is arguably also not great UX but it's also not rocket science to calculate that. Could be dealt with with a few examples.
see also my comments in 💬 Page cache isn't invalidated Needs review
kristiaanvandeneynde → credited berdir → .
I added a note, but honestly, with a module about your size, don't worry about it too much per my note. It's a small performance optimization that's mostly beneficial for modules that have lots of functions (specifically some core module with a lot of legacy code like locale). For bonus points, move that builder function to a static method and then you can remove your .module file completely.
What you *do* need to be careful with in 11.2 is the requirements hook. That too is now supported, I'm not 100% sure how that behaves exactly in 11.1 vs 11.2. Make sure you test that when converting and keep in mind that you have to convert that when adding the flag.
I also updated the original change record to avoid overselling the importance of this, it was written before we learned what the main reason for the explosive memory usage was and reference the various new related change records in 11.2: https://www.drupal.org/node/3490771/revisions/view/13843793/13993134 →
I think this looks good. I agree that this should be removed as it's a relict of a time when the rss feed was hardcoded and not a view like it is now.
This does not remove any RSS capabilities in core, just a block that displays a hardcoded link to a path that might or might not actually be an RSS feed. 📌 Deprecate RSS usage in core Active would be about removing that, which I disagree with as commented there.
Please remove the update function. we don't know how those views were adjusted, we don't own them, we shouldn't attempt to change them automatically. Existing sites can add this themself.
> > An alternative would be to grab the target entity's cache tags and invalidate those. Now that the entity storage is using a proper cache internally, we could do that.
The entity cache, static or persistent does not use cache tags. It's a 1:1 relationship, we do not add cache tags as that would require an extra cache tag lookups for every cached entity you want to load. You can invalidate it, like you could already before, but I'm not even sure that's necessary or useful, as AFAIK nothing is added to that entity on load.
> The only thing I'm slightly concerned about is hook_entity_update() and similar entity class methods as people may use that to trigger certain things when an entity is saved and said hook does not run on a simple cache tag invalidation. So if people do something there that does not somehow get cached (with the entity as a dependency), we might be in trouble.
I think the primary example for this is still pathauto, which creates/updates aliases. Everyone who creates aliases based on group assignment of an entity will be affected by that as their aliases will no longer work. (Side note, 💬 Get a token of a node's parent group to create a pathauto pattern Needs review has 100+ comments and followers, I think that's mostly for pathauto integration).
If you just remove this, this will affect the workflow for a lot of people. But I'm also well aware that this is a very expensive solution to trigger pathauto. One option might be to explicitly call the pathauto API in that case. There might be fancier options, but anything hook/event based is always a question of who implements the hook for whom, and personally, I think a factor is how common a module is. everyone uses pathauto. I can't add dozens of integrations to pathauto, but it's feasible to add a service/module check and call \Drupal::service('pathauto.generator')->updateEntityAlias($entity, 'update');
But once we get to D12 (or 13), it'd be more convenient not to have to do get($field_name, FALSE) to get a NULL return, when it seems likely that the NULL return is the desired behavior most of the time. Doing this would mean that very no uses of get() need to change now, but in the major change, only usages of get() that have explicit exception handling would need to be changed to get($field_name, TRUE), and other existing usages of get() without explicit exception handling would not need to change.
That's all true. switching the parameter would avoid the ", FALSE" in the future and it's very likely a now or never thing, because once we have it, switching the default is going to be much harder.
But the deprecation for it is pretty awkward and the possible impact for it is also bigger. ignoring our own deprecation messages is something we very much try to avoid (it's a slippery slope. Recovering and cleaning up deprecations in core from around 8.0 like entity.manager took _years_) and it makes it extremely hard for possibly affected code paths to be notified about it.
For me, it's either default TRUE with D12 deprecation or default FALSE with D13 deprecation. default TRUE means we can properly document and expose it earlier and personally, I'm willing to pay for that with the ", FALSE". But if others disagree on that then I won't insist. I do think we'll need to extend the deprecation to D13 though for the other version.
Verified that this is a clean move with --color-moved git diff (git dm alias on https://www.md-systems.ch/en/blog/techblog/2011/08/22/git-aliases. yes, using a blog post from 2011 as a documentation page).
the only function left in common_test is the weird theme alter that has a separate issue to decide on moving or deleting it.
Yes, but we have 📌 [PP-1] Explore converting Install hooks to OOP Active for that, I'm suggesting we close it as a duplicate of that. The remaining bits in .install aren't quick fixes, but there is nothing actionable here that we could use. it only did requirements and that's done now with classes.
That just moves the menu link, but when you click on it, you still jump back into the local on admin/people I'd assume.
It solves the main issue, but it's also pretty confusing. But changing the path and removing the local task would be a much bigger change.
So, lets wait for maintainer feedback whether or not that's a change they want to make.
There's a third option of just removing the link. That's also how the "Role settings" tab works. Which IMHO too is in the wrong place, but that's obviously a core issue.
so, 3 options:
1. Move the link to admin/config/people (current MR)
2. Remove the link (like core role settings right now)
3. Update the path of the route, remove local task (biggest change)
Ah, you added this to the change record:
> The default behavior will change in Drupal 12.0.0, where $throw_exception will have a default value of FALSE, so calling get() with an invalid field name and without a second argument value will return NULL. In addition, the method signature in the interface will change to include the $throw_exception argument, as well as a string typehint to the $field_name parameter.
I think we don't need to do that and keep it like that. That's a bigger change that's more likely to affect code. I think if we do that, we'd need to make it a D13 deprecation, but this seems to complicate things quite a bit.
As for 11.2 vs 11.3, I think the BC impact of this is trivial, nothing in core subclasses this method and it should be very rare in contrib/custom too. Without the D12 change, risk to callers is also basically zero as nothing changes unless explicitly requested, and it's quite useful, I plan to RTBC that once the review comment is addressed and then we can maybe still get it into 11.2 before beta.
Reviewed.
getValue() is still a major DX issue, so I think this is still kind of valid, keeping of for now. Renaming getValue() is something I'd love to do, but I'm also aware about the impact that would have.
For more context, this was originally added by #591794: Allow themes to alter forms and page → in 2009.
This might sound confusing as olivero isn't that old. But it was just renamed, I guess a few times. In 2009 it was originally added as garland_drupal_alter_alter.
Originally, this was a test to make sure that themes can implement every alter hook, with a made up hook name only used in that test. This was changed, theme alters only work for specific, explicitly called alter hooks, and the test was extend in #2228093: Modernize theme initialization → and lost its original purpose already kind of then.
IMHO, we have test coverage of test themes implementing actual alter hooks and we could just drop that hook and that part of the test, but I'm also fine wit just moving the hook to test_theme or something.
Rebased
Fixed line length
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.