🇨🇦Canada @Charlie ChX Negyesi

The custom attributes were added because I confused the attribute constructor parameters with the hook parameters. It's not a viable idea. That's on me.

Now that we know the documentation stays in api.php their disadvantage shows: they always will be controversial, which hooks get separate attributes and which don't and how many? Trying to decide how entity hooks for example should be sliced will be a lot of fun. And another controversy is this very issue, we spent so much back and forth and there's still new voices and discussion to be had.

For api.module, not having custom attributes is easier since it always needs to support #[Hook('form_foo_bar_alter')] so if it doesn't to support FormAlter on top, that's easier.

I have no problems with fixing this issue by deleting FormAlter and Preprocess attribute classes.

Then let's do this. The name was originally suggested by larowlan and now we have buy in from catch and quietone. It was ready before, the rename was simple and automated.

So do we have an agreement on HookDocumentation then?

No, that's premature, let's wait for quietone.

That's two core committer votes for HookDocumentation so it's done. I reset back

There's nothing to do. While leaking memory in long running PHP processes is, alas, all too frequent there is no need to be long running, the queue is perfectly fine to run one process for each item -- possible even in an parallel.

@sher1 thanks for you report, could you edit your comment and attach the list as a text file please? The issue is becoming very long with these super long comments.

ghost of drupal past → created an issue. See original summary → .

ghost of drupal past → created an issue.

I can trace this if needed but I will bet you anything the problem is Drupal\Core\Command\InstallCommand not flushing caches after install -- this could be hardwired in the command or added to install_tasks.

Another very long Slack discussion. First the agreement reach is the documentation remains in api.php for the time being.

This, however, circles back to the original issue which PREFIX/SUFFIX attempted to address: how does api.module recognize the documentation for #[FormAlter('filter_admin_overview')] can be found at hook_form_FORM_ID_alter?

There was immediate agreement there should be at least an attribute on the FormAlter attribute class itself which helps with this.

While it would be possible to add base_form_id to the FormAlter constructor and use the new attribute like

#[HookPattern('form_alter')]
#[HookPattern('form_FORM_ID_alter'), ['form_id]]
#[HookPattern('form_BASE_FORM_ID_alter'), ['base_form_id]]

Then a method marked with #[FormAlter(base_form_id: 'node_form')] could be linked to form_BASE_FORM_ID_alter.

The complexity doesn't seem to be worth it, the documentation for form_FORM_ID_alter and form_BASE_FORM_ID_alter is nearly identical, merging them seems to be the better road forward.

Briefly we considered an #[Implements] optional attribute at the implementation point as a modernization of the current Implements hook_foo() docblook but if it's always required then it's too boilerplate-y and if only occasionally used then the confusion on when it should be used and when not would be a challenge.

Thus, we converged on a simple solution: just use HookPattern , nothing fancy and merge the documentations where needed. It will be rare, note how hook_entity_query_alter is careful with its underscores so the matching is not ambiguous.

I checked the original issue and one significant change from the original intent is the doxygen was meant to be on the class constructor and not the class itself. This might or might not help. I committed the change to move there (but it's easy to revert).

It's entirely possible I intended to move over to the constructor because I thought we could transplant the arguments to the constructor and we clearly can't do that, the constructor is defining the params of the attribute not the hook. I am all too often wrong and too stubborn to realize. Thanks for keeping me straight.

But since DocumentsHook now targets a method, it could be used anywhere where documentation is convenient.

My suggestion would be to put it on EntityStorageBase::invokeHook() as an example. And where such a method doesn't exist, write one. The amount of boilerplate seems to be the same: one function currently vs one method factored out solely for documentation purposes. One issue certainly is @return -- if we were to factor out \Drupal::moduleHandler()->invokeAll('field_info_max_weight', $args) into a method that would return a list of integers while hook_field_info_max_weight returns an int. That's a problem. Maybe @return-hook? if phpstan can define custom tags can't Drupal do the same?

Or maybe only use DocumentsHook on methods when it's convenient and keep hook_foo_bar in *.api.php alive. For consistency, marking those functions with DocumentsHook would be nice.

This, however, would completely separately DocumentsHook from this issue and so if the decision is to go in this direction then this issue could go back to just ripping out prefix/suffix and then DocumentsHook can be resolved in a separate issue.

The feedback has been addressed where it was relevant. Parts of it, I believe was made before the whole doxygen was read so they are not relevant.

ghost of drupal past → created an issue.

As I was extending on the documentation of the attribute I realized prefix/suffix is not needed at all.

The rector rule already has the battle tested code to convert something like form_FORM_ID_alter to a preg so we can just add the name of the hook documented here and be done.

$parts = explode('_', $hook);
$isUppercase = false;
foreach ($parts as &$part) {
  if (!$part) {
    continue;
  }
  if ($part === strtoupper($part)) {
    if (!$isUppercase) {
      $isUppercase = true;
      $part = '[a-z0-9_]+';
    }
  }
  else {
    $isUpperCase = false;
  }
$hookRegex = implode('_', $parts);

Yes. The core tenet of the proposal is this: there's no known advantage of final over @final while there are certainly downsides so let's always use @final.

Could you please elaborate on what advantage final offers that @final does not?

> can we make it compatible with the object way of doing things,

It's Turing complete, nothing stops you :P

Can is not the problem, want is. It's a bit painful to do it.

It is there because it is patterned after AsEventListener, the class level could already be deprecated and removed in 12 , it's not really needed.

Ah no, not linking won't work because it's not just api.drupal.org which would make this mostly a core concern -- but it's every other tool out there too. Then it doesn't need framework review because things no longer break.

I can tell you with authority that linking an attribute to its defining class is working with api module because I wrote the attribute integration for api module :) 🐛 Attributes are not rendered when showing the source code Active

Now, that's just attributes in general not hooks. But this change would mean that api does not need to do anything else. Right?

Hook specific behavior, once again would be linking a method marked with #[Hook('form_filter_admin_overview_alter')] to the class page of FormAlter where you can find the api documentation, much like on the current page. My battle plan was to take all PREFIX and SUFFIX pairs from classes extending Hook, form a massive preg and match. You can not do this with $hook = ($form_id === NULL) ? 'form_alter' : 'form_' . $form_id . '_alter'; that's not something a simple parser can understand.

But, if the moving of api documentation is conditional with changing #[Hook('form_filter_admin_overview_alter')] to #[FormAlter('filter_admin_over')] then it just works by the nature of FormAlter being a class which links to defining page. As you can see on the test, AttributeTest is already a link: Nothing hook specific needs to happen. This does make the life of the api maintainer easier for sure.

It's a decision core leadership needs to make: is it acceptable that while #[Hook('form_filter_admin_overview_alter')] works in Drupal it will not link to the documentation page on api.drupal.org?

📌 [PP-1] Make doctrine/lexer:^3.0 compatible with \Drupal\Component\Annotation\Doctrine. Active is a great example of why using final is bad.

DocParser was written for Lexer 1 when token was an array, 2 added an object with ArrayAccess support and 3 dropped this support finalizing the move to object. "Luckily" the class is final so there is no easy way to add ArrayAccess back for the short time annotations are still supported before everything becomes an attribute. (I will crosslink this issue from the @final issue as a textbook example of why final is bad.) Also, why they dropped ArrayAccess I can't even imagine, it was four one line methods.

Recommendation: do not upgrade. Let this be until it's removed.

If that's not desirable I guess I can write a rector rule to convert every array access to an object access unless the variable is called $metadata and pray it works.

The original reason was, I thought we need to be able to link #[Hook('form_filter_admin_overview_alter')] to hook_form_alter documentation which will live on the class doxygen of FormAlter.

But maybe that's unnecessary? Just link FormAlter directly and while #[Hook('form_filter_admin_overview_alter')] will continue to work in Drupal , it won't work on api.drupal.org and that might be just fine if hooks get converted to their respective attribute when the doxygen is moved. Yeah. It might just work.

It's my fault for not realizing this sooner. Yes, that can work certainly that makes api.drupal.org implementation indeed a lot easier if we don't parse hook names and only link on attributes. But that is a decision far above my current paygrade.

I would put @final to the end I know the documentation currently calls for annotations to go there but since annotations are being removed, this is not a biggie. Doctrine can pick them up even if there is something after.

It's worth noting how this came to be because, well, it's not like this code is AI written to hallucinate interfaces and yet a web search quickly shows this interface only seems to exist in these comments. How odd. Is there perhaps more to this? No: ListDefinitionInterface was added in 2013 in #2047229: Make use of classes for entity field and data definitions → and got renamed to ListDataDefinitionInterface in 2014 in #2002134: Move TypedData metadata introspection from data objects to definition objects → still within the 8.x development cycle.

If I had the powers to change issue metadata which I don't then I would change the title to

Use @final to define classes that are NOT extension points

I've had to do a lot of deep debugging of the form and render APIs whilst working on Experience Builder.
Out of this I feel I've gotten a very deep understanding of how they work

Want a hug?

Whatever is composer is doing they need to roll it back, upstream usually uses security as an excuse to violate semver but neither 2.8.7 nor 2.8.8 is marked as a security release. What is even the point in semver if something like this can happen?

My two cents: already the security team has the policy to mark unmaintained projects unsupported. Spraying a probabilistic series of PHP tokens into git is not maintainership.

But, it's up to the security team whether they share my concern of being overrun.

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.