@godotislate

Resolved some of the test issues, but some function test failures still remain.

For the MR removed all the deprecation ignore entries except for "%Since symfony/validator 7.4: Support for passing the choices as the first argument to Symfony\\Component\\Validator\\Constraints\\Choice is deprecated.%", even though this issue is technically about one. They're all closely related, so it might be difficult to address them all completely separately.

After that, still left to do is to double check that changes to certain constraints don't break BC, and adding test coverage.

Instead of what I said in #6, I tried bringing over the bulk of MR 12291 to see what deprecations it might fix and what tests are breaking.

One major change from there is change the new ConstraintBase class not to pass options to the Symfony Contraint class parent, like this:

public function __construct(...$args) {
    if (!empty($args) && array_is_list($args)) {
      @trigger_error(sprintf('Passing an array of options to configure the "%s" constraint is deprecated in drupal:11.3.0 and will not be supported in drupal:12.0.0. Use named arguments instead. See https://www.drupal.org/node/3522497', get_class($this)), E_USER_DEPRECATED);
      parent::__construct(...$args);
      return;
    }

    $groups = $args['groups'] ?? NULL;
    unset($args['groups']);
    $payload = $args['payload'] ?? NULL;
    unset($args['payload']);

    foreach ($args as $key => $arg) {
      if (property_exists($this, $key)) {
        $this->$key = $arg;
      }
    }
    parent::__construct(NULL, $groups, $payload);
  }

Will investigate the test failures as it seems that might not completely work.

Did my best to update the title and IS per the new direction in MR 13675.

godotislate → changed the visibility of the branch 2066993-phpstan-rule to hidden.

do we think that we should only allow certain file types to be linked via these stream wrappers

If so, should the list be a container parameter or similar that can be overridden? .webp, .jpeg are missing from the example list, and it's possible there are unforeseen use cases.

There's handling for that value setter in the other MR I did on the original issue, similar to what you have so far. One thing I found was the need to be able to know in constraint factory createInstance() that the value setter thing happened in config manager create(). I'll try porting over some of that work piece by piece here and see how much it helps with the deprecations.

Is the 10.6 MR good to go to RTBC?

Interesting that Existence doesn't have the HasNamedArguments attribute, but seems to work all the same. I have vague concerns that ValidSequenceKeysConstraint does not semantically inherit from Existence the way Required and Optional do, but the code reuse convenience overrides that for me.

lgtm.

godotislate → changed the visibility of the branch 3490713-alternative-to-drupalservice to hidden.

I think the IS might need updating under "Proposed resolution," but otherwise this looks good to me. Concern about BC was addressed.

godotislate → changed the visibility of the branch 3490713-alternative-to-drupalservice-POC-conversions to hidden.

godotislate → changed the visibility of the branch 3490713-alternative-to-container-get to hidden.

Yeah, property hooks would be a follow up once we have a new major branch.

One question on the MR.

Also, similar to 📌 [meta] Use property hooks to instantiate services from service closures Active , this might be something to use property hooks for once on D12.

I wonder if that ddev 8.5 version is a little old, because the __sleep and __wakeup deprecations were reverted per #3548971-5: Deal with PHP 8.5 deprecation of __sleep()/__wakeup() → .

Thanks @benjifisher for the review and to you, @rkoller, and @simohell for looking at this in the usability meeting.

Were you able to discuss the question in the "For usability review" section in the IS? #12 raised a question about data integrity problem or perceived data loss because of the behavior not to show the content of a formatted text field on entity view when the field's text format does not exist. I proposed one solution: on entity view, display a message or markup to users who have access to edit the field value that the format is missing.

Updated the IS and downgraded to Major since 🐛 Deleting a Media view mode should not result in the deletion of an entire text format Active was fixed via 🐛 Config entities implementing EntityWithPluginCollectionInterface should ask the plugins to react when their dependencies are removed Needs work

Have a couple of questions on the MR.

I was also wondering that since the notSupported()/isSupported() are only called from LayoutBuilderEntiyViewDisplay::buildMultiple(), that the arguments to those methods should be notSupported(EntityViewDisplayInterface $display) and isSupported(EntityViewDisplayInterface $display), since we already have the display entity loaded (as $this). This would save loading the display entity again, since entity view display entities are not statically cached.

If the expectation is that these methods might be called other places (in contrib/custom code) where the display has not been loaded, then it's fine.

Some of the work in MR 12291 for 📌 Passing a $options array to constraint constructors is deprecated, use named arguments instead Postponed may be relevant.

Some of the work in MR 12291 for 📌 Passing a $options array to constraint constructors is deprecated, use named arguments instead Postponed may be relevant.

Some of the work in MR 12291 for 📌 Passing a $options array to constraint constructors is deprecated, use named arguments instead Postponed may be relevant.

📌 Since symfony/validator 7.4: Support for evaluating options in the base Constraint class is deprecated. Initialize properties in the constructor instead. Active , 📌 Since symfony/validator 7.4: Passing an array of options to configure the constraints is deprecated, use named arguments instead. Active , 📌 Since symfony/validator 7.4: Support for passing the choices as the first argument to Symfony\Component\Validator\Constraints\Choice is deprecated. Active likely cover a lot of the same ground as this issue, maybe broken up into smaller pieces.

All questions addressed, lgtm.

MR is pretty straightforward and overall looks good.

I had a couple questions about whether followups need to be created for a deprecation and PHPStan issue.
Also, it looks like two of the Symfony regressions have been addressed, but the third about the ampersand is an inconsistency in the HTML spec and won't be "fixed" by Symfony. Is there any adjustment to Drupal tests needed?

Those tests are passing, but I'm not clear why some low level Unit tests seem now to be failing?

Looking at the previous build, it looks like the failures were on a composer integration test, so it could have been a temporary network issue. Seems like the latest build went fine, other than 3 functional javascript tests failing. Two of them seem directly related to the new linking functionality. The third one is a performance test, and that could be an intermittent failure that just needs a re-run.

One idea to address #125 is introducing a memory limit/memory % threshold to the LRU cache instead.

I was about to update the CR https://www.drupal.org/node/3139212 → to reflect that memory management has been removed from MigrateExecutable, but I just read CR https://www.drupal.org/node/3512407 → associated with 📌 Use LRU Cache for static entity cache Postponed :

Sites running migrations on some (free) hosting plans with a memory limit of, say, 128MB may now get the following error.

"Memory usage is now @usage (@pct% of limit @limit), not enough reclaimed, starting new batch"

This is because the memory is no longer manually freed up whenever we do a memory usage check.

There's a chance then on hosting plans with low memory that migrations will OOM and fail now. I wonder if something should remain in MigrateExecutable to inform that memory is getting low and to reduce entity.memory_cache.slots.

Rebased and updated phpstan baseline.

Since we didn't do any deprecation here, linked https://www.drupal.org/node/3554746 → to 📌 Deprecate passing options array to all constraint plugin parameters Active instead.

MR 12291 for 📌 Passing a $options array to constraint constructors is deprecated, use named arguments instead Postponed should have a lot of the work needed here, so changes would need to be transferred to an MR here after the original issue is in.

Postponed on 📌 Allow plugin service wiring via constructor parameter attributes Needs work per #2

godotislate → created an issue.

Stubbed a follow up 📌 Deprecate passing options array to all constraint plugin parameters Active . Re #21 about a plan, maybe it can look something like this:

First, carry over most if not all the work from MR 12291, including

• Deprecate passing anything other than an associative array to the addConstraint() methods
• Deprecate passing an options array to any Drupal constraint plugin class
• To handle this with BC, introduce an abstract class ConstraintBase with a constructor that uses the spread operator, so it can take either named arguments or the options array.

Then later, in Drupal 13(?):

• Make changes to all Drupal Constraint plugin classes so that their constructors have arguments for all properties
• Figure out what to do with the ConstraintBase class at that time (if it's still needed or if it needs changing)

godotislate → created an issue.

Blocking on 📌 Handle serialization and unserialization of service closures Active .

Blocking on 📌 Handle serialization and unserialization of service closures Active .

OK, did the fallback thing after all. So even if someone creates a closure without getSerializableServiceClosure(), as long as they put the ServiceClosure attribute on it, DependencySerializationTrait will handle it. People should definitely use the method if they can though, because that saves an unnecessary instantiation of the service just to make the class serializable.

Also added a bit more test coverage.

Made minor tweaks the IS and issue title. Might also need a plan on how to break up follow up work, if it needs to be broken up at all.

https://git.drupalcode.org/project/drupal/-/merge_requests/13610#note_61... for 10.6 is ready.

@catch re: #42 I think the 11.3.x commit might not have been pushed. Maybe it happened during the Gitlab maintenance period?

godotislate → changed the visibility of the branch 3544994-handle-serialization-and to hidden.

Fixed the PHPStan issues, but tests are failing on the 10.6 MR.

Updated a couple form classes to use the new service closure method to test how they worked. It occurs to me that form classes might not be the best use case for service closures, since anywhere the form class is loaded likely will call the form processing code that uses the services, but I thought a form with AJAX might be the best demonstration of the class being serialized. I meant to do only one form to start (esponsiveImageStyleForm), but I noticed there are no FunctionalJavascript tests for that form, so I added another.

I did not do the fallback thing I mentioned in #20, and also left the attribute in place.

Attempted a backport to 10.6 on MR 13610 using doctrine/annotations 1.14.4, but ran into some phpstan issues. Will have to loop back later to investigate.

10.6.x is locked to doctrine/annotations 1.14.4, so should the Drupal forked classes in 10.6.x be copies of that version instead of 2.0.2?

doctrine/annotations is still in the composer.lock, which is causing test failures on HEAD 🐛 PHPStan job in error Needs review .

Made changes per MR feedback. Removed the deprecation warning as well.

One thing I wonder if we need to do here: in ConstraintManager::create(), there's code to convert $options not passed as an array to an array with a value key. This worked because when passed as an $options array to the plugin constructor, if the plugin classes returns something from getDefaultOption(), the value at the value key gets set as the value for the default option.

This doesn't necessarily work with named arguments, because $value might not be a constructor argument. I did some handing for this in MR 12291, by adding an internal boolean flag value to the $options array to indicate that options were not originally passed as an array, so that they shouldn't be passed as named arguments. Nothing is breaking in core tests, but I wonder if we need this possibly for contrib/custom code use of the Symfony Constraint subclasses. Or maybe it's fine to leave to a follow up?

Do we need any tests here now?

Re #16, I think reducing scope here to minimum to get tests passing with the Symfony deprecation ignore removed makes sense, and handle everything else in follow ups. I'll do my best to get that done today.

Updated MR 12268 with a small refactoring. Still WIP for some tests of the logic in ConstraintFactory, and this needs a CR.

godotislate → changed the visibility of the branch 3522497-deprecation-poc to hidden.

It occurs to me that since there's a lookup map for closures to service IDs, we don't technically need the ServiceClosure attribute. (My original idea was to pass the service ID into the attribute, but that's not possible since attribute parameters can only be constant expression.)

On the other hand, the lookup map is the part of the solution I like the least. And maybe in _sleep() there should be a fallback where any closure that has the attribute but isn't in the lookup map somehow that the closure gets invoked and then the service ID can be looked up from the reverse container.

I think that since 📌 Convert theme hooks to OOP Active is in, this is unblocked now. The MR needs refactoring since it was done on the event-based hook collector.

Yes, originally I tried having a separate trait that both AutowireTrait and DependencySerializationTrait could both use, but then there were complications about where the tracking array would live, and then if a class used both AutowireTrait and DependencySerializationTrait, I think there might be a conflict with the method being declared twice.

I think in the AutowireTrait issue, we can do something like if (method_exists(static::class, 'getServiceClosure') { ... }. It might be best to rename the method to getSerializableServiceClosure() in that case, so it's less generic and less likely that a different method with the same name exists in the class.

If there are plugins, forms, or controllers that would never be serialized, then technically they wouldn't need to use the method, and if they do need to be serialized they likely would need to use the trait to make serialization safe for their service properties anyway, so I think that works out OK.

I was thinking of relying on the search block, but there's probably a lot of value in finding a form class or something that actually gets serialized, so I'll see if I can find a suitable one.

Re #10. Discussed with @longwave on Slack, and best way forward here to get this in for 11.3 is to reduce scope to address only the Symfony constraints for now (MR 12268). Can split off using named parameters on all constraints to a follow up.

We should also try to include our own deprecation message somehow as well, since most people using constraints do not instantiate directly via the constructor but through addConstraint(), so the message telling them to use named parameters instead is not that helpful.

I will try to work on 12268 either today or tomorrow.

OK, MR 13584 has a POC of the approach I mentioned in #14. The idea is:

1. Introduce Drupal\Core\DependencyInjection\Attribute\ServiceClosureclass that can be applied to functions/closures
2. Create a new static method getServiceClosure(serviceId, container): Closure on DependencySerializationTrait that returns a closure that returns the service associated with the service ID. The attribute is applied to the closure
3. Use spl_object_id() on the closure to create a key in a static array in DependencySerializationTrait that maps to the service ID. So basically there is a map of a unique ID for the closure to the service ID, and this is done so we don't have to invoke the closure to see what it returns in __sleep
4. In __sleep(), loop through all the properties that are closures, and check for the ServiceClosure attribute. For every closure that has the attribute loop up the service ID associated with the closure's unique ID in the map, then track which properties map to which service IDs as closures. Unset the entry in the closure ID to service ID map
5. On __wakeup, repopulate the closure properties using getServiceClosure()

OK, so I realized #12 isn't great, because constructor parameters aren't class properties unless the class is using property promotion.

I do have another idea, though, basically adding a method in DependencySerializationTrait that generates the closure with an attribute. Then in __sleep, check whether the closure has the attribute. It'll be easier to explain after I have a POC, which I think I can knock out later tonight.

An alternative that I just thought of how to identify service closures with DependencySerializationTrait would be to require an attribute to be applied to any constructor argument that is a service closure. For classes that are autowired, we could use AutowireServiceClosure, and maybe that's fine to use even if classes implement their own create() method? Then on serialization, we'd use reflection to identify which the property name and their service IDs.

Re #10: I thought about this when working on the MR, and I think we're introducing an interesting DX quirk. The way I see it, the new class would only be used in plugins implementing ContainerFactoryPluginInterface or other classes implement ContainerInjectionInterface. So for now, it'd just be the search block we're working on in 📌 Replace SearchBlock service properties with service closures Active . We can also make AutowireTrait use the ServiceClosure class instead of \Closure in #3554337: Support the AutowireServiceClosure attribute in AutowireTrait to inject service closures → .

Services would continue to use the Symfony code that injects \Closures, and I think that's fine-ish? Are there cases where services themselves are being serialized? If so, maybe it'd be necessary to override how service closures are injected and use the ServiceClosure class in the Drupal container classes or compiler passes, but that sounds a bit complicated. But if we don't, it'd be on developers to keep track that \Closures are being passed in to service arguments and ServiceClosures are passed into plugins, etc., which is the DX quirk I mentioned.

Because of above, I'm not sure which approach is better. How service \Closures would be identified in DependencySerializationTrait has its drawbacks as mentioned in #6.

Gave the service closure class a shot on MR 13575.

Thanks for the reviews.
I wondering now if this should be postponed on 📌 Handle serialization and unserialization of service closures Active ?

Stubbed the follow up #3554337: Support the AutowireServiceClosure attribute in AutowireTrait to inject service closures → per #14.

godotislate → created an issue.

Added the BC for the class properties. I genericized the __get(), but then didn't do a trait because the properties aren't really deprecated, since we'll be bringing them back via property hooks in D12 with minimum PHP 8.4 support in 📌 [meta] Use property hooks to instantiate services from service closures Active . So the __get() will only be a thing for this short window of time.

s it possible to add the #[AutowireServiceClosure(...)] attribute and delete create() here?
There needs to be a separate issue to support that. It was on my todo list to create one, but I haven't gotten around to it yet and thought it would be best to land this and 📌 Handle serialization and unserialization of service closures Active first.

Also, does the CR need before and after examples?
For the class's constructor arguments deprecation? I'm not sure how that would go.

There was this note in A that I'm not sure about: language switcher blocks includes the current query string, so that would always bubble up to dynamic page cache as a cache context.

That applies when the block is not placeholdered.

I have updated the IS to show the consolidation to what was formerly option "C", which combines the use of CacheOptionalInterface and enforced placeholdering.

Fixed the logic in onDependencyRemoval a bit and rebased.

Tagged for usability review to evaluate whether something like #25 is needed and updated the IS to note that as a remaining task.

I wonder if it would make sense to show on entity view a message that the formatted text field can not be displayed because of the missing filter format. This message would be limited to users with a specific level access, perhaps edit access to the field? Though I think that could be something to be done in a follow up while the data loss concern gets addressed here first.

Added the CR: https://www.drupal.org/node/3554070 → . It's a bit verbose, so any edits, suggestions, or corrections are welcome!

One difference that remains between the hook and event system is that events can stop propagation, which the hook system so far can not. Though as @catch mentioned on Slack, it could be something added to hooks if wanted/needed.

One comment about the Reorder/Remove etc functionality that maybe is worth documenting in the CR if it's correct. I don't know that we need a specific follow up for it, because ✨ Support hooks (Hook attribute) in components Active should handle it eventually.

Re #197: I'm not sure the new APi from 🐛 Config entities implementing EntityWithPluginCollectionInterface should ask the plugins to react when their dependencies are removed Needs work will help that much, because Drupal\filter\Entity\FilterFormat::onDependencyRemoval() already loops through all the filter plugins on module uninstall and removes them if possible. The problem is that only disabled filter plugins are removed, because it's a security risk to remove enabled filters from a format. And there's the FilterUninstallValidator that runs before onDependencyRemoval(), and the validator prevents a module from being uninstalled if any of its filter plugins are enabled in formats. During config sync, module install/uninstall happens before config objects and config entities are updated, so even the incoming configuration has those plugins removed, the validator does not know that.

Possibly another reason in favor of allowing arbitrary parameters in plugin attribute constructors (sparked by #3524377-50: Allow to skip OOP hooks and services for modules that are not installed → ): since we use named parameters, this makes adding parameters in the future more difficult for BC, because a contrib plugin trying to support multiple versions will hit a fatal error passing a named parameter that does not exist in the older version.

Allowing arbitrary named parameters now might make adding any defined parameter later a bit easier, since contrib/custom modules would only need to reorder the parameter list.

Having a non-existing attribute class is just fine. Providing a named parameter to an existing attribute class that doesn't exist is a fatal error (this will be fun on plugin attributes too).

I think there have been some struggles with PHPStan in contrib projects trying to support multiple versions, where adding an ignore a non-existing attribute class for the older version causes PHPStan to complain about an unnecessary ignore in the newer version, so it's not completely painless. But yes, that's nothing compared to fatal errors.

Off-topic, but speaking of plugin attributes, I wonder if being forward compatible with future named parameters is a good reason to go forward with allowing arbitrary parameters as a solution for ✨ Allow attribute-based plugins to discover supplemental attributes from other modules Active .

Re #77, the discussion about standard browser normalization behavior to CRLF and JSON:API started in #3226410-27: CRLF causes mismatch between JS and form maxlength validation → . The concern is that JSON:API submissions with data containing "\r\n" will not be normalized to "\n", resulting in data being stored differently depending on submission method.

I think some of that discussion might be under the assumption that application/json submissions are normalized to CRLF the same as multipart/form-data, application/x-www-form-urlencoded, or text/plain. It seems like that might not be the case? In which case, the line endings for text content submitted via JSON:API might vary based on the OS where the content was authored?

🐛 Config entities implementing EntityWithPluginCollectionInterface should ask the plugins to react when their dependencies are removed Needs work is in, which resolves 🐛 Deleting a Media view mode should not result in the deletion of an entire text format Active and removes one vector of (accidentally) deleting filter formats.

Almost forgot: is there explicit testing that the base theme(s) hook implementations are invoked as expected?

I think implementing an invokeAll() method in the them manager would be a nice to have, but probably would work for a follow up if needed.

A couple small suggestions to add return typedefs to a couple new protected methods, not sure how I missed those before. I would not say they're a blocker though, so feel free to apply suggestions (or not) and self RTBC.

Just wanted to make sure to note 🐛 Deleting a Media view mode should not result in the deletion of an entire text format Active is a duplicate at this point and should be closed with credits transferred here.

Made changes per #87 to the MR and updated IS and CR. This is ready again.

pretty straightforward. lgtm

Didn't have time to look at this again this weekend, but should be able to look tomorrow.

Some comments on the MR.

Well BlockFilterTest passes now, so I guess it is just super flakey, even locally.

Tried fixing the tests by adding #[RunTestsInSeparateProcesses].

There's one test failure remaining that is weird and is also happening on my local against HEAD, so it's likely unrelated.

Drupal\Tests\layout_builder\FunctionalJavascript\BlockFilterTest::testBlockFilter
Failed asserting that actual size 3 matches expected size 2.

core/modules/layout_builder/tests/src/FunctionalJavascript/BlockFilterTest.php:110

Added to the new kernel test and rebased. Nothing else has changed, so I think it's fair to leave in RTBC.

Since 📌 Slowly, very slowly start OOPifying the render system Needs review still needs work, rebased this and back to NR.

I compared the Drupal\Component\Annotation\Doctrine\* classes and to Doctrine\Tests\Common\Annotations\* 2.0.2 classes and confirmed that the code is essentially identical.

BC for exception looks good. Also, if this is backported to D10, I think the CR alone is enough without any BC.

LGTM.

lgtm.

1 small suggestion to remove constructor docblock. OK to self-RTBC after that.

There's work started in 📌 Replace SearchBlock service properties with service closures Active to use closures in order to inject services lazily into plugin classes (the search block, in this case). I wonder if it makes sense to expand on 📌 Allow plugin service wiring via constructor parameter attributes Needs work to accommodate the use of the AutowireServiceClosure attribute as well. In which case, does it make sense to do this first and do the reflection at discovery time, and then add autowiring for service closures? Or can the two tasks be done independently?

Note that to use service closures in plugin classes 📌 Handle serialization and unserialization of service closures Active will need to land as well.