🇨🇦Canada @Charlie ChX Negyesi

it's not the actual data records, it's field instances, some fields have text_processing set to yes, some has set it to no, the system has no idea what to migrate to , open field ui in d7 and rectify the situation by setting all of the text procesisng yes if that's what you need.

--- a/core/authorize.php
+++ /dev/null

The people who quietly work on removing ModuleHandler::add* love this quite a bit. Please go ahead and make our lives (much) easier, thanks.

Just cheering you from the peanut gallery: the installer has write permissions to settings.php, doesn't it? if so then why not put a flag in there which causes an exception to be thrown in DrupalKernel::initializeSettings

in other words if a stray request causes the container to be mangled then dont let stray requests in at all

but then again, of course, i might be totally wrong

(please continue not crediting me)

Please file a feature request for that; it needs to go on ModuleHandler and not the event dispatcher.

The plan is to introduce subclasses of Hook and move the doxygen from api.php there. There are nuances around dynamic hooks, that discussion is at 📌 [PP-1] Determine how to implement Form Alters with attributes. Active .

Hrmmmm yes that could be restricted. I have a feeling I wanted to based on how the code reads but I didn't, sorry. It's quite a bit of a fizzbuzz problem :) so there can be many ways to express this. Here's one

    $subPathname = $iterator->getSubPathname();
    $extension = $fileInfo->getExtension();
    // Under src/ only consider php files which are also under src/Hook.
    $inHook = str_starts_with($subPathname, 'src/Hook/');
    $isDir = $iterator->isDir();
    if ($isDir && ($subPathname === 'src' || $subPathname === 'src/Hook' || $inHook)) {
      return TRUE;
    }
    if ($inHook && $extension === 'php') {
      return TRUE;
    }
    if (str_starts_with($subPathname, 'src/')) {
      return FALSE;
    }
    // Otherwise traverse subdirectories as long as they are not submodules.
    return $isDir ?
      // glob() doesn't support streams but scandir() does.
      !in_array($fileInfo->getFilename(), ['tests', 'js', 'css']) && !array_filter(scandir($key), fn ($filename) => str_ends_with($filename, '.info.yml')) :
      in_array($extension, ['inc', 'module', 'profile', 'install']);

Yes, hook-to-OOP went in a single go. However, that contained no functional changes and while the MR itself was over 25 000 lines it was just shuffling code around automatically and a very small amount manually and even that small amount was split into six patches. Zero functional changes. (The problems being found are coming from the new module system not the conversion. People have abused ModuleHandler in truly creative ways I couldn't have possibly expect.)

This is absolutely amazing work.

For follow up, definitely not to hold this one up: Can we remove the container from services? I guess that's a big meta. There's 🐛 Cache bin names should be set from service tags, not the service name Needs work and 📌 Replace lazy service proxy generation with service closures Active and then there's the KeyValueFactory and LazyContextRepository and there's an unused container.trait service and ... Anyways it'd be nice to untangle the resulting circular dependencies.

HookCollectorPass currently indeed loads module which loads includes as well but it parses files return by the iterator to avoid reflection on those functions, who knows whether this is best practice but that's what it currently does so you need the iterator to return every include file wherever the module author has put them.

beware, include files can be in subdirectories

This modernization is amazing.

However, I think it only makes consequences of memory leaks significantly less severe. But still, it looks like something leaks memory and maybe that leak should be found and plugged? If it's php, it has a memory leak detector when compile with --enable-debug. Maybe someone armed with such a build could run a drush si?

nikic/php-parser uses token_get_all or in php8 the newly introduced PhpToken but both of those call the Zend C function tokenize_common so that's pretty much the same. if the problem is in the Zend tokenizing system then changing the call chain to it won't help.

I hope this is not offensive but at first sight this reads as if sites were running on "Drupal unicef", whatever that may be.

if people want to go ahead with this -- and I am not saying they either should or they should not, I absolutely have no opinion, I do not event want have an opinion -- then, if someone wanted the opinion of an old ghost then #142 is the way because then it's possible to add a reflection based fromRenderable somewhere (ElementInfoManager would be my guess) so that form alter does not need to deal with arrays either. Otherwise, people would need to learn both syntax to be able to debug code.

After commit please delete https://www.drupal.org/node/3442349#comment-15877121 →

@quietone You could move this issue to the imce queue perhaps because that text is coming from https://git.drupalcode.org/project/imce/-/blob/3.x/src/Plugin/CKEditor5P...

So yes, someone needs to make a decision whether

1. No class/trait/interface checks. Require explicit dependencies and maintain less core code.
2. Middle ground: immediate classes checked with direct class_exists & friends
3. Whole hog. Make DrupalKernel::$classLoader public readonly, use $kernel->classLoader->findFile() to find any classes/traits/interfaces being depended on, read 'em, parse 'em and recurse with a static cache.

I have no opinion which one is best, I merely outlined how #3 could look.

And/or use statements could easy be parsed for non-existent modules.

/me listens to https://youtu.be/6fgudq4CcH8 for vibes

Thanks for coding what I have dreamed about for some time now -- or was it a nightmare? I haven't been this torn about something for a long while. But hey! luckily I no longer have even an informal leadership position or whatever so the burden of deciding whether to go down on this path is not on my shoulders. I will post a few thoughts which cause or allay fears.

1. nikic/php-parser is an amazing work and most importantly it's true to the PHP source code because it's generated from it. Of course, one part of the generating code, that being phpyacc, has one of the most hair raising or most badass comments I've ever seen, I can't decide which: "How does it work? I don't know. I just ported the code until it worked correctly" (Anthony is awesome, always has been, pity his interaction with Drupal was brief). But, despite it has not seen a single commit for four plus years, the code generated by phpyacc has indeed been proven correct. rector depends on php-parser and it's successful. By now, I have faith in php-parser. Phew, was that hard to admit :) (Now of course kmyacc itself has been dead for quite some time now and PHP is using Bison not kmyacc but let's not go into those woods. Let's just be thankful it worked out.)
2. This needs all the caching goodness catch is cooking because userspace parsing is not a speedy adventure especially not when NameResolver gets involved. But that cooking is going rather well so we can put aside all the performance concerns that stopped the hook oop patch being built on top of php-parser -- the original issue still has some traces of the proof concepts I coded for that.
3. (string) $attribute->name. This is ... naive, not in any negative ways of course. It just doesn't consider the absolutely wild selection of PHP features attributes support. While https://github.com/nikic/PHP-Parser/blob/master/doc/component/Constant_e... of course exists but it needs helpers for resolving constants (and other things as described under Unsupported expressions and evaluator fallback) which very well might appear here would require some work.... this is one area where I am happy not to work on this or indeed the wider effort to move all attributes (or even just hook attributes) to php-parser. If the decision is to stay with this naive implementation, which is, again, totally fine, it at least needs to be precisely documented to only support literal scalars, no constants, no property fetch expressions, none of that, kthxbai.
4. hasMissingClassDependencies is again a bit naive, this is still not some harsh criticism it's just this topic has been giving me worse nightmares than what follows a late night visit to Taco Bell. A depends on B depends on C where C doesn't exist , this code parses A and runs class_exists(B) which happily throws a Fatal error: Uncaught Error: Class "C" not found. Yay. Is this a real world problem? Does this lead to the requirement of a soft-class-exists-based-on-php-parser? Maybe, what do I know, I know nothing any more.

Please carry on. It's awesome to see taking shape.

ghost of drupal past → created an issue.

Supporting magic naming instead of or in addition to tagged services is a good idea, thanks. But I don't think it merits a separate issue, indeed it should be discussed here:

1. support tagged services
2. support magic named classes
3. support both

Implementations wise, this is once again a minor tweak to HookCollectorPass. In this case, the filter iterator needs a minor adjustment: the iterator should iterate all of src instead of src/Hook but only return php files if the subPath starts with Hook or the file name ends in Hooks. This is not a problem, much as it was trivial to support tagged services the same is true for magic named classes.

No Symfony magic can avoid the problem outlined -- catch however is solving it with caching. Indeed, the less Symfony used, all the better as we have just seen they are incapable of keeping even the most basic semver. Of course, that's just my two cents, of course someone will eventually refactor the entire HookCollectorPass to use registerAttributeForAutoconfiguration and then no one ever will be able to debug the ensuing problems -- I know, I read the entire code flow for the latter to evalute for the hook oop patch, the complexity is horrifying. But, at least, it'll use Symfony, yay! But I digress.

Sorry, I am not quite sure what you mean. The patch changes from extension to decorator yes but how does this break things...?

Thanks for the rapid fix, I like it but I can't RTBC it because I suggested the heart of the change so it would be somewhat RTBC'ing my own code. Maybe others don't quite like the succinct version and want core to elaborate on it? I dunno.

Please commit to 10.x too.

Another example of the problem https://www.smartsheet.com/customers/stegh as you can see here Learn more online: <a href="https://www.stegh.on.ca/ "> causes no problems at all in a browser but PathValidator blows up because Symfony is garbage.

don't forget field_formatter_third_party_settings_form as well

Hrmmmm, I will update the CR to talk about ::invoke the original IS did it just didn't make it into the CR -- in short, those are an anomaly too, those are not really hooks :) and luckily there are very few of them.

To be on topic. Sure, adding a service tag to indicate it's a hook and then adding those classes into HookCollectorPass is certainly a possibility. Let's think aloud what that would take, first it needs an optional $container passed to collectAllHookImplementations (optional because ModuleHandler::add calls it). Once that's there code should read something like:

        foreach ($container->findTaggedServiceIds('drupal_hook') as $service_id => $tag) {
          $container->getDefinition($service_id)->getClass()
          foreach (static::getHookAttributesInClass($class) as $attribute) {
            $this->addFromAttribute($attribute, $class, $module);
          }
        }

That's nice -- but there's no $module. That information just doesn't exist and while for service YAMLs it could be persisted with a little effort -- DrupalKernel does index $this->serviceYamls by $module even if it doesn't pass it on -- services created by providers do not have this information.

I am guessing you could make $module optional in addFromAttribute and grab the module from the class name:

    if ($hook->module) {
      $module = $hook->module;
    }
    elseif (!$module) {
      $module = explode('\\', $class)[1];
    }
    if (!$module) {
      throw new WhateverException;
    }

this will work for Drupal modules and when it doesn't, well, just specify $module in #[Hook]. Most of the time it is irrelevant anyways -- invokeAllWith needs it because it calls $callback($listener, $module); where $module is cheerfully ignored in invokeAll and indeed in almost all code using invokeAllWith but still, that signature can't be changed without serious upheaval and so something needs to be supplied there.

Indeed let's move to a separate issue, the fix is as simple as $settings_form[$module] = ($settings_form[$module] ?? []) + $hook( that a trivial bug / missing feature it's not worth changing main CR over a small bug. Like, of all hooks only field_widget_third_party_settings_form/field_formatter_third_party_settings_form is affected and it won't be affected for long.

The hmia patch did not remove the multiple implementations feature, at least not intentionally, if it's missing that's a bug, it is collecting into $this->implementations[$hook->hook][$module][$class][] = $hook->method; and the moduleImplementsAlters property is only used to determine module order. By all means it should work. I would recommend running \Drupal::getContainer()->getParameter('hook_implementations_map')['field_widget_third_party_settings_form'] from drush php and checking what's in there. If you see both of your classes then the problem is in either ModuleHandler or the closure used by EditFormDisplayEditForm::thirdPartySettingsForm (it's using closure $hook, that's one possible problem down). Truth to be told, I don't think either nicxvan or myself have tested invokeAllWith by itself which is what's used here but at the same time invokeAll relies on it so the chances of it being broken is quite low. But, it's software, and even worse, it's software written by me, so it being broken is not as impossible as one might desire.

As for supporting the hook attribute in any service, that's not a particularly hard problem, it's just a performance/memory nightmare as you'd need to reflect every class and method defining a service to see whether they have a #Hook attribute. Of course, it's not my call but if it were then I'd split Rodrigo's problem into a separate issue and postpone this one on 📌 Add a core FileCache implementation that is database-backed Active . Just a suggestion.

Yes, sorry, I confused which entity is which. This looks good to me, thanks so much for the through tests and the quick resolution of all.

Thanks for the quick patch!

I suggested checking whether $entity->_referringItem exists first before cloning because I was afraid of the performance hit here.

ghost of drupal past → created an issue.

I made a rather general fix, it's always the same problem we have been struggling with for years now. Hopefully this one is a more workable one.

Thanks for the excellent reproduction instructions. They really are great.

$entity->_referringItem can't be two things when $entity is the same object. $node is kept around in a static place (MemoryCache) and so when it's rendered twice the _referringItem will get overridden. This causes the problem when bricks preprocesses the rendered first field item $entity->_referringItem points to the second field item and things go wrong in a hurry.

This gets real interesting real quick because we have no way then to find the rendered field item from the render array?

I will ask around.

Congratulations, you found an entirely new kind of error :) I expected a lot of things but such an array was not among them. Please confirm that was $class in TypedManager::createInstance as returned from $data_definition->getClass();

Could you please dump similarly $data_type ?

Oh we cross-posted. OP, if it's production then you could use something like

$output=ob_start();
var_dump($class);
file_put_contents('public://log.txt', ob_get_clean());

and download log.txt from https://example.com/sites/default/files/log.txt or whatever your public files path is. It's going to be very small.

I strongly suspect the problem occurs when a config schema has a type which points to a class which is no longer present on the system. AFAIK in normal operations config schema is not validated and so this doesn't cause a problem. If I gather correctly what's going on with D11 in this regard, this will be more of a problem there. Anyways, TypedDataManager::createInstance doesn't check for existence before using it, it only checks for empty class. OP needs to add a var_dump($class) there and share.

If I am correct it's a nonexistent class then core needs to be more resilient: AFAIK TypedDataManager::createInstance needs to throw an informative exception and ArrayElement::hasTranslatableElements() should set a property so that parse() can catch the exception and armed with the knowledge it's parsing for hasTranslatableElements() it can safely ignore the failed element. Or, if that's deemed to be too much bother then system_post_update_add_langcode_to_all_translatable_config needs to catch the exception at least so it can continue. Is there a mechanism to inform the user from post update? Telling them to investigate config X because it failed would be good.

I was initially against the move to PHP5 because I felt private/protected limits extensibility needlessly and I felt the underscore in classic var $_foo expressed our intention this should not be used is enough. It only took 18 years for the PHP team to agree with me and drop setAccessible from reflection in PHP8, essentially turning private/protected properties into advisory.

Similarly, final should never be used. Instead, there should be some advisory saying "warning, this is not part of BC and if you extend it then you are on your own". Guess what? That advisory already exists at https://www.drupal.org/about/core/policies/core-change-policies/bc-polic... → , six years ago larowlan noted hook implementations are not part of BC, it is well established policy. So final achieves absolutely nothing except limits extension. It's a language misfeature and it should never be used. It's really that simple.

In this specific case I imagine classes would extend the hook class to reach the helper methods of the class and mark themselves as a decorator at the same time. Concerns raised:

1. Isn't it still going to find the original implementation in the original module? -- no it won't because it won't be an event listener, the decorator moves service tags (this got documented in Symfony last month).
2. What if two different modules want to customize two hooks on the same class? -- be my guest, decorators work for that.
3. What if the base class adds a method which the decorator doesn't call? -- these classes are not part of BC, that's a you problem. Really, who cares? Crippling runtime just because some distant future might -- or, you know, might not -- break something is just a bad idea. That's my entire point.

Drupal used to be extensibility first. It's upstream that is downright hostile against extensibility. Don't be like upstream.

api module support is progressing. We have generic attribute support just needs a test -- this has been so for weeks now, I have been helping nicxvan with the Oscar patch instead of api work -- and we have a proof of concept for #Hook support in specifics.

Once 📌 [PP-1] Determine how to implement Form Alters with attributes. Active is done we can write a very simple Rector rule which splits hook docs onto relevant attribute classes.

As far as I am concerned 📌 Investigate ModuleHandler::add Active is the most urgent here, it needs to be gone as soon as possible, that method is a complete lie now as it only works with procedural hooks, it can't work with anything else and so it subtly blocks progress.

Certainly. I said "if core wants to go that way" and outlined how it could look, what would it take in core and in rector. I am not calling any shots.

That's what I am suggesting, it should be a whole lot easier since the rule would only remove superfluous strings from Hook attributes. It will not change anything behavior wise, it's free, no test failures, no nothing. It's just less strings. Not to mention how easy it is to write the rule. It doesn't matter whether it's extra types or not. It could look at fieldWidgetCompleteTestFieldWidgetMultipleSingleValueFormAlter, convert it to field_widget_complete_test_field_widget_multiple_single_value_form_alter check the first arg to Hook and see it's the same, so it deletes it. If it is not the same, the rule does nothing.

It would also skip ckeditor5PluginInfoAlter for reasons outlined above, it's the only eligible Hook implementation which is followed by a number.

Well. I think an optional empty hook attribute with the method name converted to snake case as the default hook is okay. It just shouldn't be mandatory. And yeah, it's not usable for either angle of multiples but that's also ok -- it's a sensible default after all, nothing more.. It'll cover a lot of cases and when it doesn't it's very easy to do more. Change $hook to optional in Hook and in HookCollectorPass change:

  protected function addFromAttribute(Hook $hook, $class, $module) {
    if ($hook->module) {
      $module = $hook->module;
    }
    $hookValue = $hook->hook ?: strtolower(preg_replace('/(?<=.)[A-Z0-9]/', '_$0',  $hook->method));
    $this->moduleImplements[$hookValue][$module] = '';
    $this->implementations[$hookValue][$module][$class][] = $hook->method;
  }

So simple ;) but really, this is just 1 line change besides factoring out $hook->hook into $hookValue.

We can't override the method in the sandbox extension because it's final,

Y'all seriously need to consider adding the composer patches plugin and start patching out the final brainrot from upstream.

In that case use priority PHP_INT_MAX.

nicxvan is right: the new system can't support such eval'd code and it's perhaps easiest to decide runtime instead. But what if someone really, really, really wants to define hooks dynamically? It's doable: HookCollectorPass merely automates the following three things when it finds a method marked with the Hook attribute but it's totally doable manually.

1. Register the class as an autowired service (with the classname as the service id):

Drupal\foo\FooHooks:
  class: Drupal\foo\FooHooks
  autowire: true

2. Add a kernel.event_listener tag on the definition:

$definition->addTag('kernel.event_listener', [
  'event' => "drupal_hook.$hook",
  'method' => $method,
  'priority' => $hook_data['priority'],
]);


3. Record which module this listener belongs to in the container argument hook_implementations_map:
<?php
$map[$hook][$class][$method] = $hook_data['module'];
// ... later
$container->setParameter('hook_implementations_map', $map);

Note you can add as many hooks as you want on the same class/service and even the same method.

Also note the Hook attribute is not used after this.

So if you do not want to decide run time which entity type should fire your service then you could use a service provider to iterate your entity types and add the right tag directly on the field_encrypt.process_entities service method decryptEntity and record it in the map:

class FieldEncryptServiceProvider extends ServiceProviderBase {
  public function alter(ContainerBuilder $container) {
    $map = $container->getParameter('hook_implementations_map');
    $definition = $container->findDefinition('field_encrypt.process_entities');
    $class = $definition->getClass();
    $method = 'decryptEntity'; // perhaps this should be a class constant
    foreach ($container->get('state')->get('field_encrypt.entity_types', []) as $entity_type) {
      $hook = "entity_{$entity_type}_insert";
      $definition->addTag('kernel.event_listener', [
        'event' => "drupal_hook.$hook",
        'method' => $method,
        'priority' => 0,
      ]);
      $map[$hook][$class][$method] = 'field_encrypt';
    }
    $container->setParameter('hook_implementations_map', $map);
  }
}

You could keep the current code for pre-Drupal 11.1 codebases and use this for Drupal 11.1 and eventually drop the old one.

> I would assume that api module can't properly pick up #Hook methods

A proof of concept for that already exists but we decided it's better to properly support attributes and then special case from there. Attribute support branch is up at [#344963], we need a test, I will write one when I get to it.

I'd like to get #Hook api support out by the time D11.1 lands. We will see whether time permits.

Since this is RTBC I went ahead and removed the relevant section in the change record so people don't start using it.

Please continue not crediting me here there and everywhere.

ghost of drupal past → created an issue.

Actually 📌 OOP hooks using event dispatcher Needs review removed this functionality. That turned out to be a mistake.

I can already see the future thanks to test issue where nicxvan is testing converting everything everywhere all at once (or, in short, the Oscar patch). The views hooks can be converted without a problem and almost all *.views.inc becomes empty and is deleted. However, these includes not only contain hooks but helpers as well.: datetime_type_field_views_data_helper, views_entity_field_label, views_field_default_views_data

So for now, 🐛 Hook info must be procedural and module handler needs to handle hook_info properly Active restores this functionality. These three helpers will need to be deprecated manually before hook_hook_info can be laid to rest.

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

I went a different route and created a views field plugin which can display a single metatag. If multiple are required, Views has the capability to add multiple instances of the same field. https://git.drupalcode.org/-/snippets/220

ghost of drupal past → created an issue.

I dunno but three failures in all test modules sounds like incredible to me. I can assist in fixing them. The current failures seems to be coming from asserting procedural implementations not actual bugs. If there are no more formatting concerns then that's near ready, isn't it? Perhaps we could fix those locally and then schedule a 24-48 hour period when no other core commits are made, generate a baseline, submit it and get all of that in a single go? Once that's done, converting all non-test should be easy shouldn't it?

Note multiline arrays require a patch against rector because for some inexplicable reason they put a final on BetterStandardPrinter. I filed https://github.com/rectorphp/rector/pull/8875 currently the class is anonymous but perhaps we should ship EvenBetterStandardPrinter.

The syntax to ignore is for older PHP versions indeed.

But nonexisting attribute classes are ignored because PHP itself doesn't use attributes. Indeed, you can even run reflection against a function marked with LegacyHook in the absence of LegacyHook class, you can even retrieve the name of the attribute and it won't blow up until you try to instantiate it. There's no reason to run a class_exists against attributes. Much the same with use. It's just a string up to the point when you try to use it as an actual classname to instantiate. https://3v4l.org/sK4If As visible from this if we wanted to make a mess we didn't need to create the LegacyHook class at all, you can just check for the hook name whether it exists or not.

This is why I said people stuck on 10.1 or something already are fine to use it or to use contrib using this attribute -- it just doesn't matter.

1. In HookCollectorPass::collectModuleHookImplementations adding a if (!$fileinfo->isFile()) { continue; } to the foreach ought to fix this
2. This is a php bug. RecursiveIteratorIterator defaults to LEAVES_ONLY and should not return directories.

The rector rule might not have been rewritten from ground up since this got committed but close enough.

It now detects whether something is a hook based on doxygen Implements hook_foo(). This is in the Drupal API documentation standards → . Here's why: parsing hooks from api.php could work for core (but it's not easy) but contrib can implement hooks on behalf of other contrib which is not present on the filesystem (this is where not easy tilts into impossible). Compare this to the core implementation of hook recognition (which also considered doxygen based at one point): for core, if a nonhook function is considered a hook, it costs us 1) maybe an entry in an array in the arguments of the proceduralcall service 2) an autowired service definition. This, while not zero, is not much. On the other hand, not picking up a hook is disastrous. There is no choice for core. For the rector rule, the price of misidentification in any direction is roughly the same, not exactly but roughly the same amount of manual work of code shuffling. So it's better to err on the side where fewer errors are found and that ought to be doxygen based detection. Core has it, phpstorm has it, ask me whether I care about the rest. Some work went into properly detecting hook_ENTITY_TYPE_insert and friends. Splitting form_test_form_form_test_alter_form_alter based on Implements hook_form_FORM_ID_alter(). can go either module name form_test, hook form_form_test_alter_form_alter or module form_test_form, hook form_test_alter_form_alter. For now the code goes with the shortest possible module name. It is a one character change to go with longest, somewhat more work to test for both and prefer the one where the modulename is the current module. I decided this work is not worth it. Others might decide otherwise. The code is GPL, have at it.

It now finds out whether the original had a return and if yes then but only then it adds a return to the legacy hook function. This is only added to contrib, core is detected thanks to composer this is easy and the legacy implementation is removed.

It now spits out different classes for different files. The files are numbered for includes. Perhaps something from the include file name could be integrated instead. However, I expect people will manually rearrange the converted code quite a bit so I am not too worried about this. If you are? The code is GPL, have at it.

As requested by berdir, the method names now have the current module name chopped off from their beginning. Yes, this has an incredibly, incredibly small chance of creating a collision in method names if the module was implementing a hook on behalf of another module and so forth. Ask me whether I care.

And yes, as we discussed in Burgas LegacyHook class needs to be added to 10.x so phpstan doesn't cry. PHP itself is fine both use and attribute usage of class that doesn't exist. It's not picky. phpstan is.

Please do not credit me on this issue. Thanks.