🇨🇦Canada @Charlie ChX Negyesi

ghost of drupal past → made their first commit to this issue’s fork.

There's a much worse problem: people now post AI generated modules which were clearly never even read by a human. The cherry on the cake is they have the audacity to opt such code into security coverage. I just reported a security hole in such and I haven't even started looking seriously. These modules should be removed from security coverage and their publisher needs to lose their security coverage privileges IMO. I doubt we can stop the proliferation of them because they will just post this garbage to github otherwise but no one should for the security team to deal with slop.

@see NestedArray::unsetValue()

ghost of drupal past → created an issue.

About api module, that's my bad, originally I said I do not know how it will fall out but api already has a "class hierarchy" option which we can reuse.

This is bogus so far. PantheonDocumentWithSmartComponentTest::setUp calls documentTraitSetup which calls parent::setUp.

And I do not think any twig nodes are directly instantiated by this module.

Just grab the three relevant config files from standard/config/optional and install them programatically.

I tried to understand how runtime works and I tried to contribute my fresh understanding: I submitted https://github.com/symfony/symfony-docs/pull/20909 and created https://app.infinitymaps.io/maps/mnJJ9FR4FND. I hope this helps someone.

My worry about unit tests would be performance but I am so often wrong in what is useless micro optimization and what is legit worry.

I simply refuse to add more bloat. But, if it's required, I can just won't fix this, it's not so important.

Added one.

Converting an existing test is problematic: migrate tests are suitable but those are prophecy. My bad: when we added them (more than a decade ago, mind you) prophecy was fresh and it seemed to be the future. It seems by now this didn't turn out to be the case.

ghost of drupal past → created an issue.

ghost of drupal past → created an issue.

ghost of drupal past → created an issue.

Sorry for the misunderstanding: I thought you are converting hooks by themes.

Given the standing issue about removing ModuleServiceProvider I would advise to be cautious about introducing another magic named class

This issue and 📌 \Drupal\Core\Entity\Query\Sql\Tables causes extremely poor performance when using MariaDB and filtering on multiple relationships in JSON:API Active IMO needs to be consolidated.

https://www.drupal.org/node/2008758 → the last commit to https://www.drupal.org/project/vdd → was seven years ago. It's dead, Jim.

Can we do without the test as per 🌱 [policy, no patch] Better scoping for bug fix test coverage RTBC ? Is this something that needs a test?

As far as I am aware including from a vfs:// url requires allow_url_include on which is system level setting and is widely considered a security hole so that's probably not a good requirement for running tests.

However, you could include from phar:// and taking over the phar stream wrapper is doable, I linked two issues where Damien and me have did it before, neither made it into core however. But being a little hacky in tests maybe doesn't hurt as much as it would on the hot path.

Every content entity type extending ContentEntityBase will have an integer ID field (see baseFieldDefinitions), so only content entity type classes that do not extend that base class could implement string IDs.

    $fields = parent::baseFieldDefinitions($entity_type);
    // In order to work around the InnoDB 191 character limit on utf8mb4
    // primary keys, we set the character set for the field to ASCII.
    $fields['id'] = BaseFieldDefinition::create('string')
      ->setLabel(new TranslatableMarkup('ID'))
      ->setReadOnly(TRUE)
      ->setSetting('is_ascii', TRUE);

Guzzle already has the solution.

/**
 * @final
 */
class Client implements ClientInterface, \Psr\Http\Client\ClientInterface

For more modern code we could even create an attribute. If we are really enterprising we could create an attribute through Psr and then world peace is just one small step away after that.

NodeHooks1::queryNodeAccessAlter née node_query_node_access_alter exists and is tested by NodeQueryAlterTest. Also, among others ✨ Grant query level access to own unpublished nodes Active has the opposite claim. So I think more details are needed here on how to reproduce this.

Adding a EnforcedResponseException::redirectToUrl factory would also make it easier to redirect from inside the form method of an entity form. Returning a response is not possible from that method, it leads to a fatal error so already an EnforcedResponseException needs to be thrown there.

Actually document what's there to be done

🐛 Aggregation of JavaScript files can cause redeclaration Active

ghost of drupal past → created an issue.

ghost of drupal past → created an issue.

Let me restate #89: while the letter of the law doesn't include service providers the spirit of it certainly does. The way I read https://www.drupal.org/about/core/policies/core-change-policies/bc-policy → basically everything that is like a "herd" implementation of an interface is not BC, specifically listed are plugins, paramconverters, entity handler etc.

It only makes sense TBH. You should only be interacting with the interface not the specific classes.

Since uasort hits the comparer function more than once for each element https://3v4l.org/lYob3 I thought I would recommend assembling the array ahead of the time and then use natcasesort instead, something like

      $result = array_map(function ($a) use ($field) {
        foreach (explode('.', $field) as $property) {
          $a = $a[$property] ?? NULL;
        }
      }, $result);
      natcasesort($result);

but I can't recommend this because I do not understand the current code at all. I am looking at a07a4ba051 from #2942569: Sorting nested properties of config entity queries does not work → and I still don't understand it. Say there are some third party settings you are sorting on, one config object doesn't have them then one half of this loop becomes NULL the other half becomes a something and then you are trying to sort NULL to something? What is even the desired sort outcome of this? I will link this question to the authors of the current codebase to see whether we can do something.

it would probably help to provide an example of how one might want to extend a hooks class.

this has been answered in #8

In this specific case I imagine classes would either or both a) extend the hook class to reach the helper methods of the class b) perhaps tweak or completely override a hook method in both cases they would mark themselves as a decorator at the same time.

.
yes b) is being addressed in 📌 Hook ordering across OOP, procedural and with extra types Active

Also, on a much wider scope: why does this matter? You can't predict the future, you know final closes the door and gives you nothing in return so why do it?

Let me ask again, more short and more precise: as of Drupal 10, you need to upgrade once every two years from Drupal LTS to Drupal LTS. You are planning to spend a significant amount of money to decrease the frequency of two upgrades every four years to two upgrades every five years. (If that is possible of course because there are other dependencies than Symfony , Twig for eg does not have any lifecycle policy I am aware of.)

Is this worth it?

Frequent, disruptive upgrades – Organizations and developers spend considerable time updating Drupal’s dependencies rather than improving Drupal itself.

I contest this from two angles.

One, the Smartsheet webteam only does one "upgrade all things" core+contrib session once a year on the two sites it maintains, one of it has more than 260 modules installed, the custom part of the codebase contains 350 .php and .module files in total which contain close to 30 000 lines of custom php code. It rarely takes more than a week. Is this frequent? I don't think so. We established this pattern in the D8 days and since Drupal 10 it is now possible to do it once per two years to upgrade from LTS to LTS. Is that too frequent too?

We do separate out problems or likely problems, "big" contrib changing branches like schema_metatag did in 2024 and rarely core problems. Last year it was ckeditor4 => ckeditor5. If we kept going with ckeditor4 then even more functionality would've been piled on it and the upgrade would've become even more painful.

There was a twig_capture which is highly relevant here: the module hardwired a Twig 1 class name which was deprecated in Twig 2 and dropped in Twig 3 and we didn't catch this until the drop in D10. But, here is the thing: we had the chance if we had ran Rector on this module when D9 went Twig 2. If a Drupal cycle stretched so long we went from Twig 1 to Twig 3 we wouldn't have had even a chance for a peaceful upgrade. This will only repeat in the future.

Without pouring more AI slop in this issue you need to actually substantiate the claim that a) once every two years is too frequent b) by going even further the upgrade doesn't create a massive headache.

Your other claims are AI hallucinations as well: for example, under "Inconsistent support timelines" you list "PHP or Symfony reaching EOL mid-cycle" -- it's possible I missed a Symfony EOL mid-cycle but I can't recall any. Care to give us some examples? PHP EOL is what it is, Zend provides paid PHP LTS versions, expecting the entire open source ecosystem to use it just so Drupal support can be stretched further is not realistic.

But my strongest argument is data which AI slop never is: Symfony is on a four year cycle https://symfony.com/doc/current/contributing/community/releases.html PHP is on a four year cycle https://www.php.net/supported-versions.php and as of Drupal 10 , Drupal is also on a four year cycle.

If you can see a better cycle then instead of posting more AI slop please post dates. Actual dates. How long should Drupal 11 be supported if not four years and how will that work when Symfony and PHP gets out of support because they will . Things like that.

(if you need form API assistance message chx on Drupal Slack. Response times should be below 24 hours but usually below 4 hours.)

Sure , make Drupal unextensible, who cares, already Symfony does that and we knwo Symfony is the best codebase there can be, am I right

mcdruid → credited ghost of drupal past → .

OK back to the service_providers approach it seems, our approach has failed to gain acceptance.

ghost of drupal past → changed the visibility of the branch 2910814-tagged-services to hidden.

ghost of drupal past → changed the visibility of the branch 2910814-deprecate-magic-service-provider to active.

No, it's not worth not converting, these bugs are BC only and temporary. Just because I didn't get it right at once doesn't mean we should use that as justification to make core worse.

OK. The problem was tagged services marked with servicemodifierinterace got added to the service provider objects list before depreciation message hit so those were thought to be deprecated.

(MysqlServiceRegister should be changed to implement only one of the interfaces.)

OK and after naming it back, what are you running?

Thanks! Yes, that's a recent oversight from #86. Please test again and if there are still unexpected fails, I will need reproduction instructions but I think we are good now.

> in case someone is doing something with these classes

https://www.drupal.org/about/core/policies/core-change-policies/bc-policy → basically everything that is like a "herd" implementation of an interface is not BC, specifically listed are plugins, paramconverters, entity handler etc. Also, it is not at all clear why would anyone want to, changing services has its own interface. Perhaps to reach a utility function but the only provider with a utility function is languageserviceprovider and that's not used in contrib https://git.drupalcode.org/search?group_id=2&scope=blobs&search=language... and since the functions are isMultilingual / getDefaultLanguageValue it's hard to see why custom code would need 'em , surely you know whether your site is multilingual or not. Oh and protected methods are also exempt from BC so you shouldn't be using them anyways. So: the class is exempted, the methods are exempted and they are not used for what little there is.

Of course, of course, that's what I meant. https://www.drupal.org/project/drupal/issues/3485896 📌 Hook ordering across OOP, procedural and with extra types Active is first so people don't try to order via event listener priorities.

Truth to be told, I only kept the event dispatcher because it's easier to get changes in if they use Symfony. It's truth. I wish I was kidding but I am not.

One thing I should note: service providers should not, in general , do $container->get('foo') they should only do getDefinition/findDefinition, I noted at the end https://www.drupal.org/node/2974194 → how to get a list of modules, how to access (raw) config and encouraged settings instead.

To clarify what #83 says, if there's no rename then people won't understand why they need to tag it. But, the rename is not forced. Core does the renames to be more clear, contrib can do whatever they want. I just pushed a version where the tagged services fire first and then if any magic named classes remain then they fire and trigger the deprecation. Previously it was the other way 'round which meant if someone actually injected something in the service definition it wouldn't have worked.

Yes, of course, you and I already wrote an event dispatcher in #1972300: Write a more scalable dispatcher → it's not like that's hard to do it was reverted in 📌 Replace ContainerAwareEventDispatcher with Symfony EventDispatcher Fixed we can unrevert it :D :D

Note both autowire and autoconfigure means every class defined as a service will be loaded and reflected during rebuild by Symfony. I think this issue is completely moot in light of that.

For hooks, a performance improvement would be to pass down the container as far as getHookAttributesInClass and use $container->getReflectionClass() to save on reflecting the classes twice -- ContainerBuilder keeps the reflections in a property.

Btw the same is true for TwigExtensionPass it's just even simpler I will include it because it shows what I am talking about:

      $class_name = $container->getDefinition($service_id)->getClass();
      $reflection = new \ReflectionClass($class_name);

change to

      $class_name = $container->getDefinition($service_id)->getClass();
      $reflection = $container->getReflectionClass($class_name);

Please check the problem/motivation section of 📌 OOP hooks using event dispatcher Needs review

I upgraded https://www.drupal.org/node/3357408 → to be quite a bit more clear about what is happening and what is not and crucially removed the link to the Symfony documentation because Drupal only autoconfigures registered services -- Symfony picks classes up from the filesystem and registers them as services so it was quite a bit misleading as to what autoconfigure means to Drupal.

Nonetheless, autoconfiguration does not apply here since as that's a service pass and this runs before any service passes.

I rebased on 11.x.

1. The way this currently works, legacy first and then new service providers if their class is not legacy named. We could very easily move the new services first and only fire legacies if they have not fired as services. This indeed would allow not renaming them. However, I am for renaming to show things are changing
2. I looked at 📌 Enable service autoconfiguration in the DI container Needs review and I am terrified. How on earth did that go through? Y'all looked at Symfony documentation and just trusted it sight unseen? Performance? Any caveats? Did anyone read the code in there? How does it work? While it looks good on paper, it's still Symfony which has proven multiple times not to care that much for the scale Drupal does things. I for one refuse to use it until it's better understood. Just as an example, because for the Hook OOP issue, I did the relevant research which is completely missing here: the FileLoader::findClasses() scans the filesystem and then converts the path and filename into a classname PSR-4 style. But Drupal's YamlFileLoader does not extend FileLoader. So how does it work? I am going to unfollow this issue and any followups for mental health reasons until this resolved.

Sorry we have not answered #65 adequately. I checked the core service providers:

1. multiple providers add registerFormat calls to the http_middleware.negotiation
2. InlineFormErrorsServiceProvider does not decorate but uses extension. Since it overrides a protected class, a decorator refactoring would be somewhat difficult.
3. entire service passes are added by some providers
4. Many modules (layout_builder, mysql, node...) module provides services which require other modules to be installed.

Overall, I do not think this can be dropped.

https://symfony.com/releases/7.3 comes out in May

https://www.drupal.org/about/core/policies/core-release-cycles/schedule → 11.2 comes out in Jun

if this gets in now, a released Symfony still will predated a released Drupal

(not that I have any idea why would that be a prerequisite)

Thanks that's much better.

I filed 📌 Optimize Containerbuilder::findTaggedServiceIds() Active as related to the new super simple implementation.

ghost of drupal past → created an issue.

Please do not force api module to chase an entire chain of ancestors, always define both constants.

I tried to shorten the code by moving things into a service pass but ordering makes it not viable. This is short enough.

The tagged services branch is ready for review. I do not think it needs an explicit test, converting LanguageServiceProvider shows it works. I added preliminary code for deprecation, it just needs to be uncommented.

I added a few ideas and wrote a new issue summary.

Let's widen this.

I contacted marcoscano

ghost of drupal past → created an issue.

As always, I was curious how this came to be.

Well, there's no explanation. The camelize was added in d1e52e0089 which doesn't have an issue ID associated with it neither does the commit message "Various cleanups" tells us anything. This is what happened:

-      $class = "\Drupal\\{$module}\\{$module}Bundle";
+      $camelized = ContainerBuilder::camelize($module);
+      $class = "\Drupal\\{$module}\\{$camelized}Bundle";

And then in #1988508: Stop pretending we support Symfony bundles when we really just have service providers → it was merely kept. In that issue effulgenstia had an idea which might or might not be worth reconsidering: call the service provider simply Drupal\modulename\Services.

No need to write a new test, just stick a Drupal::getContainer() call into any module used by a functional test, just to the top of the module. If it doesn't blow up then that's good enough, comment it's the test for this issue and to move it to another module should that one get deleted for any reason and call it a day.

At this point this is a waste of effort because 📌 Hook ordering across OOP, procedural and with extra types Active will remove most

To recap what's happened: the original hook OOP patch was a half year effort (including me travelling to Burgas on my own money solely to get it done but I digress) and obviously went through a lot of revisions. Including every module seemed necessary to get tests pass. As the patch shows, by the current revision it's no longer necessary and can be disaggregated into separate includes. We just never went through this effort because it didn't seem necessary -- but of course it is. This change is also beneficial in that it allows removing the separate includes in the future as each legacy mechanism bites the dust:

• 📌 Investigate ModuleHandler::add Active , 📌 12.x Remove ModuleHandler::add and methods that use it. Active
• 📌 Hook ordering across OOP, procedural and with extra types Active
• 📌 Deprecate hook_hook_info() Active

the answer is https://drupal.org/project/tranc and now it can be closed indeed.

#17/#18 is a different issue as far as I can tell. I don't understand it but it's not the same issue, again, as far as I can see.

Everything old is new again

This code was created in https://www.drupal.org/project/drupal/issues/2616814#comment-12065522 📌 Delegate all hook invocations to ModuleHandler Fixed and then in #25 the performance question was raised, acknowledged and never addressed.

It's more relevant than ever because core just moved old procedural code into Hooks classes.

Almost all of Views code is unchanged since 7.x-3.x or even older.

And so on.

To help others a little when trying to figure out: MenuLinkDefaultForm::buildConfigurationForm() is not guilty. It rightly presumes menu links have providers because in the default storage they do have a default, it's system, it's coming from MenuTreeStorage::schemaDefinition() and the defaults in there are, not sure why but are merged into the link in MenuTreeStorage::preSave().

No, the empty provider is coming from the MenuLinkFieldDefinitions::$defaults trait, why is that empty when after save it becomes system anyways. If I were to suggest anything I would suggest changing this to system as well -- but I don't do so. This is just an idea. Please continue not crediting me.

And if I were curious, I would certainly wonder how come this error has not happened before -- even the earliest versions of ModuleHandler::getName would fatal return $module_data[$module]->info['name'];. What has changed? Now that we have a test (wonderful!) perhaps someone wants to git bisect.

Note on Method 4 for cache clear but no update

So, possible solutions are

1. Default to functions belonging to the module they are in and only if there is no such prefix match then do a module_preg to find "implements on behalf of others". This only changes behavior if there are two modules where one is a prefix of another. Currently my_module_library will be matched as hook module before my_module even inside my_module.module / .inc. After this change, it won't be. It's a change. I have no clue whether the BC policy allows for making this change.
2. Parse doxygen if the prefix problem is present. We are adding more code to a BC layer
3. Just store both splits. This means the size of the implementations container parameter grows.
4. Demand conversion for these ambiguous cases. I have no clue whether the BC policy allows for this one. We do have the rector rule and ncixvan already did core and he is converting webform now with it. That's proof enough it is working.

> The other option would be to register both/all possibilities, that would essentially keep the current behavior?

Brilliant idea, thanks that should work and should be easy to implement too, upgrade the preg_match to preg_match_all \PREG_SET_ORDER and loop it.

It's not possible to figure out whether my_module_library_info_alter is module my_module_library hook info_alter or module my_module and library_info_alter. You can change things but it'll just break some other case. The information is simply not there. This has always haunted the module system 📌 Use something other than a single underscore for hook namespacing Closed: duplicate

There is one way out: we could borrow doxygen parsing from the rector rule but only fire it if there's a prefix collision for performance reasons -- there's no need otherwise. This would require collecting modules which have a prefix collision and if the module preg hits one of those then That's the only way out. If the function name doesn't have doxygen on it then you can't tell what's up and what's down.

my dream always was to stop using hook_update_N for content and write in place migrations exactly using this plugin

a followup, of course but still