🇨🇦Canada @Charlie ChX Negyesi

Yeah, arguments: [!service_closure '@example_service_1'] is exactly the godforsaken ugly syntax I mentioned. Nah. We could and should do better: arguments: ['@>example_service_1'] and be done.

You want ServiceClosureArgument

Your problem is our YAML loader AFAIK doesn't support it

You want to add a notation like the already existing @? probably @> because it's reminiscent of fn () => and probably upstream it too -- Symfony has its own way to do it using some godforsaken YAML notation uglier than sin and waaaaay too long, too.

I pushed a small change which shows there's no need to use loadInclude now. I am not 100% on doing this but it'll go away anyways when node admin is converted to a class which seems inevitable.

I am sorry but that's not what this issue is about. That TODO has nothing to do with documentation, AttachedAssetsInterface doesn't document what $attachments can be and also given ✨ Allow additional keys in #attached Needs review I am not sure whether that TODO will ever resolve.

Ghost of Drupal Past → created an issue.

Tell us more. I mean, Smartsheet is running this code without a problem and we have multiple value fields aplenty.

I read the page you linked and solely based on how AsEventListener behaves -- which I have studied for the OOP hook patch -- I think you will be very disappointed at what autoconfigure: true actually provides with the components Drupal core currently uses: it is registered here aka in the full symfony/symfony package. twig.extension is added in Symfony\Bundle\TwigBundle\DependencyInjection\TwigExtension which does ship with Drupal core but as far as I am aware, Drupal doesn't use Symfony bundles at all.

This is not to say it is completely useless -- but it does require some work, a bit of copy/paste at least to make it really useful.

Note the FORMID constant was just a discussion in the issue, it's not found in the code yet. The attribution parameter is called form_id.

I marked the PR as non-draft.

Thanks for the meta review :)

1. It's in draft because I have no idea what that means. I am just writing patches, here (in two weeks it'll be twenty years). This new workflow is a complete unknown for me.
2. I talked with catch about a review , likely next week.
3. I will
4. Profile was addressed I thought?
5. That's part of #1 I think

Edit: sorry I didn't see the review by solideogloria I will process that too.

https://www.drupal.org/project/drupal/issues/3272093#comment-14642054 🐛 Cache bin names should be set from service tags, not the service name Needs review was RTBC but there was a complaint about how it always instantiated the service. It's possible this was not solvable at the time but now we have ServiceClosureArgument and I am 99% it is a perfect replacement for Reference here. So $factory->addMethodCall('offsetSet', [$bin, new ServiceClosureArgument($id)]); and then return $this->offsetGet($bin)();.

The latest revision of the OOP Hook patch I am working on also failed on that AjaxTest. https://git.drupalcode.org/issue/drupal-3442246/-/pipelines/164384

I just rebased this so it applies.

While I did resurrect ModuleHandlerTest it is an extremely strongly coupled unit test which has assumptions about how the implementation exactly works. In particular, ::testInvokeAll presumes addModule followed by invokeAll loads the module while ::testLoadModule asserts addModule does not load the module. There is no rhyme or reason to presume addModule doesn't load the module and so now it does. The system proposed here can't work otherwise because module_implements_alter needs to be called build time and it would be inconsistent if we only loaded the modules implementing it. Since addModule is a very rare, install time only thing, I see no harm in this change.

Oh, that's easy to solve, just put them in src/Attribute/Hook.

mstrelan , that's a wonderful idea. Custom code could still just use #[Hook] for an undocumented one-off hook if needed for quick-and-dirty -- the ability to keep the zero fuss on either caller is implementation side is of extreme importance to me (indeed that's why I jumped on this). But clean code could just use create a custom attribute class and put the API documentation on the constructor. A somewhat small change to api.drupal.org, hopefully. I presume it would still all extend the Hook attribute just set the hook name to something relevant. We could use a class constant for that, so it would become ->invokeAll(Cron::HOOK) and #[Cron] on the implementation side which would be the exact same as >invokeAll('hook') and #[Hook('cron')]. I really like this idea.

Supporting the full container.namespaces instead of module namespaces to include core is certainly easily doable.

While more tests certainly need to be added this is now green and certainly ready for review.

@acbramley that decision is not mine to make. As far as I can tell, there are two things Hux allows for and the current PR does not and both are easy to add either here if it's blocking or in a followup if desired. I do not yet see the need but I am easy to convince.

1. hooks in subdirectories -- we already have multiple classes, do we need multiple directories too? I can imagine arranging into say NodeUserHooks.php and NodeTermHooks.php but ... subdirectories? Are we really going to need them? I can write them, sure, we already have a recursive iterator, using another is not a biggie.
2. hooks as tagged services -- I think the Hook attribute on a class completely replaces this need. If I am wrong, let me know and I will add this.

Re-reading Hux after writing this was certainly useful because I quickly added the replaces functionality.

This PR has some advantages over Hux. Apologies if I missed some of its functionality.

1. Complete subsuming of procedural hooks resulting in a simplification of ModuleHandler by moving some of the code to discovery time.
2. Support for the rather complex alter ordering for multiple types. I think my support is easier to understand than the current one, even.
3. Speaking of ordering, an equivalent for module implements alter is supported -- without adding any new mechanism even. Just use service provider alters on the new container parameter.

For the future, this patch delegates the actual sorting to the event dispatcher allowing for a followup which adds before/after functionality benefiting both events and hooks automatically.

I would punt on themes. While ThemeHandler does add the themes to the PSR-4 namespaces and so it is maybe possible to give themes a similar treatment I would like to proceed in as small chunks as possible to make it easier to get this in. Please note DrupalKernel currently has zero to do with themes and we would need to figure out some sort of theme namespaces during container build which might raise some issues: the patch currently has failures around installation, I am afraid it make even this worse.

I posted a new version folding procedural hooks in resulting in much simplified ModuleHandler -- and the build time parts are not so bad either. We discussed with catch and just throwing in every function as a hook candidate seems a very reasonable plan as already there are not that many functions which are not hooks.

A few tests around install and update are not passing yet, the rest are happy.

The only unrelated change I made is renaming core_field_views_data to views_field_views_data. But since that's a hook implementation I doubt it's a problem. If not desirable, we can revert -- it will work now but when I made the change ModuleHandler::invoke was stricter, requiring $module to be a $module and $hook to be a $hook. Since then I dropped this for more backwards compatibility.

I am all in for adding before / after into the attribute, I see zero challenges here, HookCompilerPass could run the (already documented) min/max as needed, but could we punt this into a followup? My hand is up, I will write it, no problems. I just would like to proceed in minimal slices so we can proceed in parallel after. Likely the conversions made in this issue shouldn't be merged either, they are there to ferret out any problems (and they did).

Because while I see no technical challenges, the exact syntax likely will take quite a bit of discussion to figure out -- is it before/after a module? a list of modules? a list of class-method pairs? how do we indicate I want to be first/last? what if two does say want that? what if the ordering is not doable? A wants to be after B, B wants to be after C, C wants to be after A, opsie. So there's a bit to discuss on that front.

1. I am not sure how this works. It's entirely possible you can already drop hook: -- if it just calls the constructor then you already can. I pushed a commit doing this.
2. We could add an AlterHook attribute extending Hook which adds _alter and ... that's it. So Hook('foo_alter') and AlterHook('foo') is the exact same. Is this acceptable?
3. Regarding procedural, this is described in the issue summary: I advocate against the complexity of adding a harness to call the old ones from the new system but of course that's just me. But I also wrote it up in #8 how to integrate them seamlessly using a ProceduralCall class and a magic __call method call whatever procedural call it is passed to.
4. Also I am not seeing the event based hooks take care of module implements alter -- this is explained in the issue summary and an example is provided in the PR. The existing service provider infrastructure is used.

But how would the implementation know which hook is being invoked currently?

They don't. This feature is for code that doesn't care. Please check node_comment_insert and node_comment_update -- they are literally the same code.

Move nikic/php-parser up from dev dependency to normal and run something like https://gist.github.com/chx/9f289ffd541701228ba98243dd7eaff7

Ghost of Drupal Past → created an issue.

Ghost of Drupal Past → created an issue.

Ghost of Drupal Past → created an issue.

> Do they specifically need to be registered as services?

Well, yes, we use the event dispatcher to fire them and the dispatcher uses services.

Note the full Symfony framework will also register every class it finds under a specified directory which is usually just App\ so that system is designed to take it. Surely by now we would if our array dumper/loader couldn't but can't see why not.

> I'm torn between using priority to be consistent with Symfony, and suggesting we use weights to be consistent with Drupal-land.

You and me, brother. I have no idea what to do here. This is up to community to decide. Code wise, it's trivial to change: change string replace priority to weight and a - sign in HookCompilerPass because the event dispatcher does use priority but that's an internal detail, really.

I neither know nor do I want to figure out how to convert procedural functions into something that the event dispatcher can fire. The entire system is designed around hook implementations being methods on objects. But I have a suggestion. Let's pretend we have a dumper which can dump the service definitions written by autowire. Then:

1. Convert the hook code to the new system
2. Dump the service definition generated and add it to service yml
3. Change the procedural call to manually call the converted code
4. Add a [#HookConverted] attribute to the procedural code. This would be ignored completely by pre-patch Drupals and this patch could change ModuleHandler to skip functions marked so.

This would allow modules to convert to the new system yet remain compatible with old Drupals without maintaining the same code twice. Ordering would need to be doubled up if this is what the community wants to do. Not my call.

Wrong issue, please disregard. This newfangled process completely confuses me. Patches were way easier.

Ghost of Drupal Past → created an issue.

It was only modulehandler that needed a closure in an earlier version because you got a circular dependency error but I got rid of it. As you can see entity type manager was already autowired, the next version which I intend to post in a separate issue won't need it. Thanks for making me try it again :)

1. We had a bit of discussion whether we want to allow multiple implementations for a module and the end result was a yes. Except for hooks that are called by invoke. We would need to document those. There are very few: mail, library_info_build, help in runtime and install/uninstall/schema/update_last_removed in install and that's about it. I am not 100% what happens to the install time hooks they might need to remain procedural? Far future problem.
2. I am introducing a new Hook attribute here. Makes sense: these are not quite events.
3. I moved the conversion of the module implements info into a compiler pass and then I realized this actually allows implements alter with zero added code because service provider alter can be used to do this. I added a PoC for a conversion, note "This module's implementation of form_filter_format_form_alter() must happen after theeditor module's implementation" doesn't quite make sense if we are allowing multiple implementations per module: it needs to be a specific method of a specific class, rather. While there's some vague OOP design concern here about how a module needs to know about what another does, well, implementation alter is already against another specific piece of functionality in another module so nothing new here.
4. The hook priority defaults to negative module weight. This way all the actual ordering is delegated to the event dispatcher. Normally the module weight is at hand, needs some adjustments around module (un)install but nothing much. Might need a separate issue though.
5. I included a real conversion of node_user_cancel, it's one of the trickier ones because it uses module handler but the example shows the power of autowire solving this.

Special named classes are only for the "implements alter" because I am just not sure whether it's doable or desirable to fire off an event in the middle of DrupalKernel::compileContainer(). Every other alter is, of course, the same pattern as hooks. Whether we want a different attribute for those, I do not know. Maybe?

You are right, these are not quite events because, after all, events have event classes and these do not.

And while using AsEventListener has the advantage of reusing a Symfony thing, truth to be told the code reuse is rather minimal here -- as I noted while the AsEventListener attribute class itself ships with symfony/event-dispatcher there's literally nothing else, it's never used for anything. Using a Hook attribute instead would have the advantage of allowing module as an attribute argument and wouldn't be a lot of code -- it seems like ~20 LoC, it's just a constructor. The registration code could rather easily autofill the module key making the runtime code even simpler (yay) but it would allow hook implementations to specify it themselves thus checking off the implement on the behalf of another module feature. Overall, it's a good idea.

Of the features listed, well, you can't mix the order of converted and non-converted code. Otherwise, ordering is possible using priority as with events. While temporarily it'll be weird to use the opposite of weight not having two different ways for events vs hooks is IMO good. My previous comment suggested creating an implements alter magic named class for the purpose of, well, implements altering the new style ones.

Conditionally included, sure, these are autoloaded classes.

I will post a new patch tomorrow.

1. Does a service with the correct attribute 'just work' as a hook too with this approach? Or would we need an extra compiler pass for that? No and -- maybe surprisingly -- I suspect the second is also no. This is a separate, much simpler issue, I strongly suspect lifting this ten lines of code from Symfony framework into DrupalKernel would cause the container to add proper kernel listener tags to services which have this attribute because all the code necessary is already included in symfony/dependency-injection but I haven't tested this yet. But, as I said, this seems like a completely separate issue to me. Could easily precede this one.
2. To facilitate implements alter we could gather all the necessary info into an array: keys are hook, class, method value is priority. Then run a new implements alter on them. I am not quite sure how the container behaves at the point this code runs so it's possible it'd be better if we used magic named classes for this purpose alone -- much like we do with FooServiceProvider -- instead of a hook_event_hooks_implements_alter or some such. Do we have experience in instantiating a service in DrupalKernel::compileContainer? I doubt that's wise, really.
3. A new service could be defined which uses the existing dispatcher class or extends it and throws a logicexception on dispatch. Could even proxy the event dispatcher, all the same. Perhaps move the new hook-firing logic in there and eventually merge modulehandler into it and drop modulehandler on the long run (DrupalCon Portland looks good :P).
4. Then using the altered array just directly run $module_dispatcher_definition->addMethodCall('addListener', [$event, [new ServiceClosureArgument(new Reference($class)), $method], $priority]); instead of tagging. I had an earlier version doing these calls. It's not necessary to add a tag.
5. I wouldn't worry much about BC, of course old hooks can't just die from one release to the next but I expect the conversion to be automated and very swift. I do not see a problem with any hook names, the conversion is: "in module foo, given a function which name starts with foo_ and has doxygen which says implements hook_ take the function name, replace foo with hook, that's the event name". The converter script doesn't need to know about Drupal: a directory containing foo.info.yml and subdirs belongs to module foo unless it contains another bar.info.yml. This is essentially what EclipseGc's code does, sorta. I just don't want to do it runtime, let's just convert stuff. I do not know much about phpstan but we have excellent people who do -- but if push comes to shove I can write it with nikic/php-parser in less than a day -- I would find out the event name and the function starting and ending line with it but do the actual conversion with just string operations. But that's just how I would do it. Name a D Day and convert core in a single commit that day. Prepare for it with test issues, of course. On that day, mark procedural hooks deprecated and drop them from the next major. So maybe introduce it in 11.1 or 11.2 and drop procedural in Drupal 12. Or does such deprecation need to wait for D12 and drop in D13? Rapid conversion alleviates the pain with having to deal with two sorts of module implements alter.

I am happy with the feedback so far, thanks catch and donquixote. I do not see any major pain points raised nor anything which would require adding a lot of code. My goal here is to keep both boilerplate and added core complexity down to an absolute minimum.

I read eclipsegc's patch. I liked it quite a bit. But I thought we could perhaps do even better.

I observed 📌 Replace ContainerAwareEventDispatcher with Symfony EventDispatcher Fixed and read https://symfony.com/doc/current/event_dispatcher.html . I searched for AsEventListener to see how it is used and observed it is not used at all and realized while it is shipped with the event dispatcher, it is only wired in the full framework here.

Inspired by all that, may I present my humble patch. Here's an example:

namespace Drupal\node\Hooks;

use Drupal\Core\Messenger\MessengerInterface;
use Symfony\Component\EventDispatcher\Attribute\AsEventListener;

#[AsEventListener]
class Whatever {

  public function __construct(protected MessengerInterface $messenger) {
  }

  #[AsEventListener(event: 'hook_entity_load')]
  public function foobar() {
    $this->messenger->addMessage('here');
  }

}

That is all.

1. Absolutely minimum boilerplate. There's no need for a service definition or getSubscribedEvents.
2. Very little Drupalism and not really new at that: the class needs to be in a Drupal\modulename\Hooks namespace and according directory -- this pattern should be very familiar to Drupal developers because of the plugin subsystem. AsEventListener is a Symfony attribute and we use it as it is documented.
3. Any number of hook classes, any number of hook methods, any method can implement as many hooks as it wants.
4. very very little code -- less than eighty new lines is added to core. alter is not implemented yet but it should be trivial based on how invokeAllWith and invoke goes. Likely it will need some performance love, too.

Of course, events registered normally using a service and getSubscribedEvents() work just as well -- this should only be necessary when autowire doesn't suffice.

Conversion from the old to the new I am sure will be done with rector at the end but may I note it could be done by search-and-replace: take "Implements hook_theme()" and rewrite it into #[AsEventListener(event: 'hook_entity_theme')]. Implements was three lines of code, this is only one so it actually decreases the size of boilerplate :D An exemption to coding standard doxygen for AsEventListener methods and classes would be required to keep the boilerplate at minimum but since hook implementations also have no doxygen this can't be a problem.

The two should be merged into one, I am not partial which stays alive. That one has a test (yay!) but this has a bugfix of some importance:

    if (!$this->requestStack->getSession()->getId()) {
      $metadata->setCacheMaxAge(0);
    }

larowlan → credited Ghost of Drupal Past → .

Ghost of Drupal Past → created an issue.

This approach is truly flawed.

There's nothing to do here, the right thing is to change the config override language.

The new tranc_config submodule of https://www.drupal.org/project/tranc → does change it to the content language on non-admin page which is what we needed.

Yes, K-V is where I would store this, we already store module version there, put the "this module is safe to uninstall" flag there as well. Please don't restrict to feature flags, there are more than a few modules which know , just by the subject matter that it will never need an uninstall hook. The system already automatically deletes config belonging to a module being uninstalled, there's more than a few that needs nothing more.

because uninstalling a module that doesn't exist on disk throws a warning/error

It's core, we can bypass uninstall validate for feature flag modules and just nuke them from config storage and K-V. Smartsheet updates do have a helper for nuking modules like that, it does work.

    EventLoop::defer(function () use ($suspension, $key, $operation) {
      try {
        $result = $operation();
        $suspension->resume([$key, Result::ok($result)]);
      }
      catch (\Throwable $e) {
        $suspension->resume([$key, Result::error($e)]);
      }
    });

I really dislike this pattern. I complained in Slack this doesn't feel like PHP to me and later when digging around for the event dispatcher issue have I found https://www.drupal.org/project/drupal/issues/1972300#comment-9216241 → which expresses the exact same concern about the same pattern.

But whether it's typical to PHP or not, it's certainly alien to Drupal.

And we have a lot of examples of what happens when brilliant people (and Kingdutch is one without a doubt) introduce patterns familiar to them.

We see it in Views which was literally written by a wizard (hi Merlin :) ) and it is still a PHP4 codebase mostly because no one has the slightest idea any more what's going in there. If you disagree, please document what items are in MultiItemsFieldHandlerInterface , thanks :)

We see it in entity query where Tables.php is an eldritch abomination written by me (sigh) and the moment I stopped working on 🐛 Entity field relationship queries for multi column field items stored in shared tables are broken Needs work the issue died.

We see it in Typed Data where the number of bugfixes per year is below one (not counting the mechanical upkeep with latest upstream/Drupal). And that's not because it's bug free.

The moment Kingdutch stops working on this, this component will die too.

This is why I object.

The plugin does not need to support the timezone, the underlying PHP structure does. If your source contains a timezone, use T in from_format and then PHP will do the right thing:

https://www.php.net/manual/en/datetimeimmutable.createfromformat.php

The timezone parameter and the current timezone are ignored when the datetime parameter either contains a UNIX timestamp (e.g. 946684800) or specifies a timezone (e.g. 2010-01-28T15:00:00+02:00).

and then DateTimePlus::createFromFormat() will read the timezone from the PHP object: $datetimeplus->setTimezone($date->getTimezone());

return !($value === NULL && ($key === 'deriver' || $key === 'provider' || $key == 'entity_type_class'));

return isset($value) || in_array($key, ['deriver', 'provider', 'entity_type_class'], TRUE);

First of all: feel free to dismiss my concern. My view on things are warped because I spend little time building from the UI, not that much writing new code -- most of my time is spent with debugging. So when I see this:

  foreach ($operations as $key => $operation) {
    EventLoop::defer(function () use ($suspension, $key, $operation) {
      try {
        $result = $operation();
        $suspension->resume([$key, Result::ok($result)]);
      }
      catch (\Throwable $e) {
        $suspension->resume([$key, Result::error($e)]);
      }
    });
  }

this makes me afraid it'll make code simply undebuggable. I have been burnt by the GraphQL module before using similar patterns. You put a breakpoint somewhere, get a stacktrace and it's completely useless because it comes from this thing which merely executes something assembled by something else and the two thing are so far removed you can't stitch it together again. I very well recognize this is a pattern used in other programming languages but that is a theoretical reassurance that doesn't help debugging Drupal code.

Using exceptions as signals in this manner is not ideal.

I would argue it is exactly what exceptions are made for: to break out from the ordinary code flow. Indeed, SubProcess shows the need for manually propagate instead of the exception automatically doing so. Look at the number of checks for skipping before and after. What use cases are made possible by adding this complexity? What problems are being solved here?

Also, I am not sure why is it called "getSkip". Let's look at events: the method is called isPropagationStopped so the analogue would be isProcessingSkipped.

If it is a performance issue could you perhaps grab the queries and post them with an EXPLAIN? Thanks!

Please provide migration tools. phpstan is awesome. the jquery once change was not. thanks.

Confirmed:

curl -s https://packages.drupal.org/files/packages/8/p2/drupal/ckeditor_selectall.json |jq -r '.packages["drupal/ckeditor_selectall"][0].homepage'

https://www.drupal.org/project/ckeditorselectall

Belongs to p.d.o though.

I really, really, really do not want to get involved in decision making but it seems this issue focuses on build time decoding.

However, SerializationInterface::encode can and is used run time. Dropping support for the vastly faster PECL emitter here is nothing short of disastrous. Please keep it. For my use case, keeping only YamlPecl::encode and throwing a not supported in YamlPecl::decode is a perfectly valid solution, cutting the maintenance burn down to practically nothing while keeping run time performance high. Thanks for your consideration.

There are two things here.

1. Updating the radio element to change #return_value will not work with forms which define singular radio elements like core/modules/views/src/Plugin/views/style/Table.php does. If I were fixing this -- but I do not -- I would indeed change Element::children but I would rather add a new #also_children key and change Element::children to take it into account like this:

  public static function children(array &$elements, $sort = FALSE) {
    // Do not attempt to sort elements which have already been sorted.
    $sort = isset($elements['#sorted']) ? !$elements['#sorted'] : $sort;
    $also_children = $element['#also_children'] ?? [];
...
      if (is_int($key) || $key === '' || $key[0] !== '#' || isset($also_children[$key])) {

and in processRadios just set it to a copy of #options. This way singular radio elements and heaven knows what else can also enjoy the fix (Table.php has machine name keys so it doesn't suffer from this but there are a lot of forms out there).

2. However, this functionality is old. Like, beyond ancient. This is so old it's not even my code, no, this is adrian's original form API, this was introduced in https://www.drupal.org/project/drupal/issues/29465#comment-341700 → how come we didn't come across this until now? Is there some unspoken expectation of radio options keys being machine name? I see a test for a less than sign for XSS purposes and so that expectation certainly doesn't stand. We certainly need much more robust testing to make sure that expectation didn't seep into theming and such. I asked on twitter and got confirmation for Drupal 6: https://twitter.com/jrbeaton/status/1747046198731538798 so it was possible to add these options for some time now and yet... I neither can recall nor can Google find any mention of this until now. Bewildering!

From #42 it would seem #27 was not clear: I suggested _symfony_routing_access to indicate a route is using symfony/Security instead of Drupal's system. I did not suggest this flag to change the behavior of Drupal's system.

I also believe both #42 and #44 needs some consider whether whether generic modules changing route access would open security holes with them.

For small changes I think #14 was the only feasible one. But, of course, what do I know? This is just my thought.

For large changes you would need to bring in symfony/security.

and eventually deprecate the ability for prepare_row hooks to return FALSE as a signal to skip a row

This was one of the warts that remained from the Drupal 7 migrate system. It was not part of the design at all.

This much, much simpler works for us.

The entity query backend for SQL -- as it does for many things -- passes range unaltered to the database query. It does so for many things. While we certainly had the intention to make the system work for any storage backend, they were not implemented at the same time: note the time gap between #1801726: EntityFieldQuery v2 → and #1846454: Add Entity query to Config entities → . Also note how hard it was to query config -- the current implementation didn't appear until #50 (!) in the latter. No wonder small details like range NULL simply was overlooked. A test for "// Add a range to a query without a start parameter" was added but, alas, this used a 0 start instead of a NULL start.

$start NULL then got documented many, many years later in #2711739: Missing summary/param/return docs for several QueryInterface methods → by copy-pasting the doxygen from Database/Query/SelectInterface::range() to Entity/Query/QueryInterface::range() however as this issue shows, not all backends implements range() this way. Indeed, in core we have a KeyValueStore and a Config backend and both does array_slice($result, $this->range['start'], $this->range['length'], TRUE); completely ignoring the fact that $this->range['start'] can be NULL. I am quite surprised more modern PHP doesn't complain a NULL is passed where an int is expected, it sure does like to complain when a NULL is passed in lieu of a string. It's possible it'll start complaining indeed in the future so if the issue ends up with allowing NULL for a start then this definitely needs to be fixed by a simple type cast.

There are multiple ways forward. Deciding is up to you. I am just an old ghost haunting the issue queues. And it's entirely possible there are yet more ways forward I missed. As this issue shows what we all know too well: I certainly can overlook the obvious.

1. Keep the behavior as is and change the doxygen to say: $start NULL has undefined behavior. This is the most BC friendly way. The only code change here would be to the array_slice() calls: add int typecasts.
2. Keep the doxygen and change QueryBase to remove $this->range if NULL is passed in. I think -- but of course all this needs confirmation from someone more knowledgable about current affairs -- this would be an API change and might be tricky to navigate through the BC policy. Once again, I think -- but again please double check -- adhering to BC policies this would take many releases: first passing in NULL would need to be depreciated because the behavior of it changes for non-SQL stored entities and instead of passing NULL to $range there would need to be a new method to remove previously set range. Then, if it is desirable to arrive back to where we are now then in the next release depreciate the new method and remove the NULL depreciation. Then finally remove the new method.

One more thing to note: while currently removing range is perhaps not a common thing to want but maybe with the alter hook finally implemented there might be more use cases for it in the future.

it originally meant "turn this render array into a plain string,

It certainly has confused this old ghost a few times when this method returned HTML instead of plain text.

Strong support for renaming it.

@longwave Replacing this code with something more modern would surely be welcome. When I ported KSES to become filter_xss WhatWG was but a year old and HTML5 didn't exist for another two years yet. And as far as I can see, this code didn't change at all in the near two decades since.

Using private properties is a rather uncommon phenomenon in Drupal core,

Thankfully. Some of my advocacy still survives then.

maybe the situation is better

How is stopping extension better? Leave that to Symfony.

Parent classes do not have expose their internal state to child classes

And then how can we fix their undesirable behavior?

otherwise they would be harder to refactor or more open for modification rather than extension (O from SOLID).

I never liked solid dry pies.

grep -c '^-.*catch' 1379.diff
0
grep -c 'if.*>shouldSkip' 1379.diff
5

that this is intentional to some extent; if we didn't mean for that to happen we could have not done that when this code was first added.

When this code was first added in #678714 theme_hook_suggestions was a special variable set in hook_preprocess. At this point variables and suggestions were changeable together simply because they were not separate things yet. The hook was added in #1751194 like this Drupal::moduleHandler()->alter('theme_suggestions_' . $base_theme_hook, $suggestions, $variables);. At this point both the hook documentation and the comment at this place only mentions the suggestions as alterable: function hook_theme_suggestions_HOOK_alter(array &$suggestions, array $variables) { -- $variables , not &$variables. The comment said: // Allow suggestions to be altered via hook_theme_suggestions_HOOK_alter(). instead of "Allow suggestions and variables to be altered". I searched the issue in question for $variable and there was not a single mention of the necessity of them being changeable in suggestions. So everything suggests variables was not supposed to be alterable, it just so happened alter takes everything by reference.

As for ThemeManager::alter , that was added in #2228093 as a copy of ModuleHandler::alter. There was no special consideration, it was just copied. The issue didn't discuss any of this either.

This is just to provide a little historical background which is all I do.

Running some git archeology

file=core/modules/menu_ui/src/MenuForm.php;for commit in $(git log --pretty=format:%h $file) ; do git show "$commit":$file|grep -n '*'|sed "s/^/$commit:/" ;done|grep :235:

reveals the file contained a * on line 235 between commits d0d3e53361 and 44f76c6bcf in 2014. The tagged versions for this period are 8.0-alpha12 and 8.0-alpha13. But, even then it's unlikely this file contained a syntax error: while even I have a hard time remembering when we wrote the menu test it certainly was included in the first test commit in 2008. And indeed manually reviewing the file shows this * was part of the doxygen, back then.

Perhaps try reinstalling Drupal 10.2.0 or tell us how are you installing it.

I reviewed https://www.drupal.org/project/drupal/issues/1801726 → and it never actually existed and we didn't catch it in time despite the dozens of revisions I did and the dozens who reviewed it. The documentation clearly shows I initially wanted to add it but didn't.

I am sorry :(

Try https://drupalusbcandmore.blot.im/upgrading-from-drupal-9-to-drupal-10

Is this valid now?

yes it is. Let me emphasize a part of the code you pasted above:

    if ($operation !== 'view') {
      return AccessResult::allowed();
    }

What needs to be documented here is you can not rely on fieldAccess alone to check whether a field is accessible by a user, it must be andIf'd with entity access.

Ghost of Drupal Past → created an issue.

The spelling errors can be skipped by adding the words to core/misc/cspell/dictionary.txt or by using a cspell:ignore line much like core/modules/filter/src/Plugin/migrate/process/FilterID.php does, for example. I am not familiar with current best practices in which one is preferred, but a) core/modules/migrate/tests/src/Kernel/HighWaterTest.php already has a Highwater ignore line b) repeated ignores seemingly are not a problem because there are multiple sourceid ignores in migrate. So: I think you should specifically ignore them instead of editing the dictionary.

I didn't have any specifics , no, I guess this is fine then, sorry for the noise.

A very wise former core subsystem maintainer taught me to introduce a helper method when some functionality is repeated at least three times.

Currently migrate Sql and mysql\Connection popCommittableTransactions extracts the mysql error code. This adds the third. All three are using subtly different checks which once again shows a helper would be helpful. Also, don't we need to loop the previous exceptions as FinalExceptionSubscriber does?

I think this should be ready. Short of an expression builder which won't happen any time soon as it didn't happen for JOINs either in the last 13 or so years I can't imagine how we could get a better solution.

I am reasonably sure this is due to https://www.drupal.org/project/drupal/issues/3072952 → please test that with latest IEF.

https://www.drupal.org/project/inline_entity_form/issues/3396757 🐛 Inline editing is broken on content that have workflows enabled Needs work

This is https://www.drupal.org/project/drupal/issues/3072952 →

Charlie ChX Negyesi → created an issue.

As catch noted in #11 to add a new argument to a public interface you need to comment it out and leave a todo, see https://www.drupal.org/files/issues/2023-12-05/interdiff_3343385_9-12.txt → and https://www.drupal.org/files/issues/2023-12-05/interdiff_3343385_9-12.txt → on how it was done for my patch.

So yeah, my approach wouldn't work for PostgreSQL after all.

Sad panda.

So either go with the suggested approach in the MR or add method groupByExtension

:(

Since we are changing the interface we could just add the modifier as the second argument of groupBy.

And a test.

One more.

Wrong patch.

Oh, there's a getGroupBy method which makes any changes to the $this->group structure not viable, I am afraid.

May I suggest a more direct groupByModifier method instead? It fixes the issue and requires minimal code. Of course, it also needs tests so I am leaving this as CNW.

Previously if you had ->groupBy('somealias WITH ROLLUP') now you will have ->groupBy('somealias')->groupByModifier('somealias', ' WITH ROLLOUP').

Drawback: if someone overwrites Select::__toString() then WITH ROLLUP gets lost without a warning.

Smartsheet also ran into this and can confirm https://git.drupalcode.org/issue/drupal-3343385/-/commit/13ca08cadc101ab... fixes the issue. However, it also needs testing.

Also maybe a better fix would be a groupByExpression method.

Either way it needs work.

I was unable to reproduce this with either 2.1.0 or 2.1.1.