🇩🇪Germany @donquixote

Let's see what others think.

It can be a good idea to replace complex inline expressions with variables, but I don't think this is such a case.
I suspect it is mostly historic reasons, to keep the #2619482: Convert all get_called_class()/get_class() to static:: → footprint small.

donquixote → changed the visibility of the branch 3495495-inline-class- to hidden.

donquixote → created an issue.

This use of randomString() looks suspicious.

    $this->drupalGet(\Drupal::service('file_url_generator')->generateAbsoluteString($directory . '/' . $this->randomString()));

For those tests with real random-generated values, it would help if these values were reported, so that hopefully it can be reproduced with exactly these values.
One idea could be to pass random values from a data provider, but perhaps there are easier solutions.

Exactly this. That was done on purpose, as we otherwise have two classes with the same name that we have to alias.

I removed the questionable commit.

In case of name clash of the class alias (e.g. attribute vs annotation) we can use other shortcuts like `Annotation\Something`, where we might import the respective Annotation namespace.

Too bad...

Namespaced classes/interfaces/traits should be referenced with use statements

Alternative is to use an alias.

An example where the namespace import is used is Drush commands, where we have `use Drush\Attributes as CLI;` and then `#[CLI\Command(...)]`.

donquixote → created an issue.

donquixote → created an issue.

Seems we are suffering this one, 📌 [random test failure] BlockCacheTest Active
And also another random test failure which happened earlier.

@mile23

Searching through Drupal 11, I see that: A) LoggerChannelTrait is unused within core, and B) therefore no services follow this pattern.

I do find it used in the following places in core:

core/lib/Drupal/Core/Controller/ControllerBase.php
core/lib/Drupal/Core/Form/FormBase.php
core/modules/layout_builder/src/Plugin/Derivative/FieldBlockDeriver.php
core/modules/jsonapi/src/ResourceType/ResourceTypeRepository.php
core/modules/image/src/Plugin/Field/FieldType/ImageItem.php
core/modules/views/src/ViewExecutable.php
core/modules/language/src/LanguageNegotiator.php

At least LanguageNegotiator is a service.
But the bigger usage is through base classes ControllerBase and FormBase, which also have other Drupal::*() calls.

I wonder if we should stop using these base classes with their hidden magic.

Patches for people to apply in their composer.json.

Actually right now it is 4.x, but perhaps 3.x has the same problem.

donquixote → created an issue.

The MR is a proof of concept for now.
I am not going to attempt to make anything pass.

I don't know if all the `@template-extends` and `@template-implements` is really needed.
Also not sure about all the `@var` docs, if that is really the best way.

donquixote → created an issue.

We want to avoid that a service gets instantiated only to reset its internal cache property.

Would that not apply only to static properties being used as an internal cache? Most of these internal caches I've seen use regular properties, meaning the service is instantiated either way.

E.g. in drupal_flush_all_caches() we call `\Drupal::theme()->resetActiveTheme();`, which does get an instance of ThemeManager, only to set `$this->activeTheme = NULL;`.
But if at the time this is called, ThemeManager had not been instantiated yet, `$this->activeTheme` would already be NULL / uninitialized.

The same would happen if we use regular event subscription to call a ->reset() method to reset an object property.

I think most of the solutions proposed here avoid this problem in one or another way.
Which is good :)

However, I would argue we could have a "default" cache for memory like we do for the DB and encourage everyone to use that cache for their simple property caching. Then there's no overhead whatsoever because there will always be something using said memory cache and therefore the service will already be instantiated.

There is overhead in reading and writing these cache items, which is always going to be more php operations than with a property cache.
All of it is just basic php operations. So it will only become relevant for caches where we read or write many small values.

If we were using the Symfony core, we would already have such a mechanism.

We want to avoid that a service gets instantiated only to reset its internal cache property.
I did a little experiment and it seems this works.

The ServicesResetter does skip lazy services.
The ResettableServicePass adds IGNORE_ON_UNINITIALIZED_REFERENCE to the service references which it adds to the IteratorArgument.
This seems to do the magic.

What I don't see is how we could use this to reset different services for different events.
It's all or nothing.

All classes and all of their methods (including private methods but excluding constructors) must be documented.

This line is far too generic.
It means we can either omit the full phpdoc on a constructor, or we have to put a complete phpdoc.

Maybe there are some parameters that we do want to document, but others where it would be redundant.
In that case we might want to omit the parameter description on some parameters but not all.

This really is about removing redundant documentation and condensing the doc section afaict.

As mentioned before, it would be useful to have a proof of concept MR that shows how this would look like in core.

@drunken monkey

On the contrary, it seems like a more descriptive parameter name could make the description less necessary:

Arguably, your example could also be read in the opposite way:
The reason why you name your parameter as "$reviewer" instead of "$account" is also the reason why you want to have a doc description.
Yes you could game the system and choose a worse parameter name to be allowed to avoid the parameter description.

It's a causation vs correlation thing.
E.g. an individual will be better off if they take medicine. But statistically, people who don't take medicine are healthier.

Personally I am not really invested in the heuristic.
I see the biggest possible benefit in reducing redundant service constructor parameter descriptions.
For other methods I don't care, I think I would rather keep the parameter descriptions.

For $container->getByClass() this is going to be more difficult, if we want to maintain full BC.

I was going to introduce a new interface ServiceByClassInterface and ConvenientContainerInterface, which would be implemented by our container classes.
But then in every place we need to check if the container we have is actually implementing that interface.
If instead we add the method directly to our ContainerInterface, any custom implementations will break which don't have this method.

----

Alternatively we could introduce a function like this:

$time = service(TimeInterface::class, $container);
assert($time instanceof TimeInterface);

so the same as above but we pass the $container object, so it is cleaner.

or

$time = ServiceFromClass::create($container)->getByClass(TimeInterface::class);
assert($time instanceof TimeInterface);

or we have something that returns a closure.

$time = resolver($container)(TimeInterface::class);
assert($time instanceof TimeInterface);

$resolve = resolver($container);
assert($resolve(TimeInterface::class) instanceof TimeInterface);

But currently a lot of the $container->get() calls should rather be replaced by some kind of autowire solution.

I would like to add tests, but not sure where they would go.

For PhpStorm you can generate meta information about services.

Thanks!
In the past I tried the symfony plugin for phpstorm which was also supposed to do this.
But it slowed everything down.

I think what I am proposing here is nicer, and more universally available.

donquixote → created an issue.

donquixote → created an issue.

I suspect none of the fails (phpstan, phpunit, phpcs) are caused by this MR.

I have a simpler approach.
I did not see how to rename the MR, and also I don't want to destroy others' work.
So I created a new branch with new MR.

Note that this issue only covers a part of the problem.
There are still other scenarios when JwtTranscoder is not fully initialized, and we are sending NULL values or accessing uninitialized property:

- The JwtTranscoder->algorithm should also be initialized with NULL, because it can be accessed before it is initialized.
- In JwtTranscoder->encode(), we need to check if $key is NULL.

For now I consider these problems as out of scope for this issue.

Rebase and code cleanup.
I still have some concerns with the branch, which I will address later.

I am attaching a patch for people to apply in their composer.json.

donquixote → made their first commit to this issue’s fork.

What about something like this:

Each parameter must be documented with a @param tag with a fully-qualified type and a description, except:

• A parameter description can be omitted, if:
  • The parameter type is a single class or interface name, AND
  • The parameter name is derived directly from the type name, AND
  • The description would not add any new information.
    (This last condition won't be checked by any code quality tools, and is up to the developer to decide.)
• If none of the parameter have a description, and the types in the @param tags only replicate the parameter type in the php function signature, then all @param tags on that function can be omitted.

I am sure this can be further improved:
- We need to find a better wording for "native php parameter type".
- The "derived from" could be more precise. The current "take the ..." looks a bit out of place though.
- For the second part, I wonder if we allow a function that has a `@return` doc but no param docs.

Also it would be interesting to see a proof-of-concept patch/PR with param docs removed from core methods and functions, to see how this would look like, and to verify that we are really all on the same page.

The proposed text still needs to be more clear:
- What exactly can be omitted: The param tag, or just the param description? What about the type?
- If all parameter descriptions are optional, and all parameters have a native type declaration, we can drop all the param tags in phpdoc? what if we still have a return doc?
- Where in the doc page does it say that a param doc needs a description? The part being changed does not mention the description at all.
- Does this also allow to drop the entire doc comment, or is it out of scope?

Sorry, I don't think this is rtbc with the wording currently proposed.

If the result is the same as the type then the doxygen is optional (see other exceptions below).

I don't think the term "doxygen" should be part of the doc page.
And, it is not clear what "doxygen is optional" means: Is the parameter description optional, or the parameter type, or the entire `@param` tag, or the entire doc comment?
The text should explicitly say this, without using the term "doxygen".

catch → credited donquixote → .

@krystalcode (#103):

So, what is the purpose of sorting use statements?

For me, the main purpose of ordering anything, including imports, is to avoid noise and conflicts in git, and duplicated entries resulting from faulty merges. These problems can be avoided if there is one canonical order that is followed by all developers and tools.

I don't like fast-forward merge.

donquixote → made their first commit to this issue’s fork.

donquixote → created an issue.

I think I was wrong in the other issue.

Actually, I think I made noise for nothing.

If I understand correctly now, procedural implementations are added as listeners with prio like -1, -2, -3 etc.
So you can always set a prio of "ridiculously negative" like -9999, to make sure it will run last, after the procedural hooks.
So, apologies for the distraction.

@catch

I think the problem with legacy hooks first, is that all of the existing modules that use hook_module_implements_alter() to run last, are currently using procedural hooks.

Actually I don't advertise for legacy hooks first.
Just for the possibility to have some new hooks run last.
So maybe we should set the priority of legacy hooks to -1, not 0, if that achieves the goal.
Then you can set yours to -2 or lower to have it run after the legacy hooks.

An alternative could be to have a dedicated event listener for the procedural hooks, with priority 0.
This way, OOP hooks can run before or after all procedural hooks.

With this, we still disrupt contrib modules that rely on hook_module_implements_alter() to run first.
But we no longer force modules to use procedural implementations if they want to run last.
The former group of modules / use cases will be smaller.

Actually..
I have concerns about the order of legacy hooks.
(nothing new, but I changed my mind about whether we should care)

The order imposed by this change puts all OOP implementations first, and legacy/procedural implementations second.

This means:
If you want your implementation to run last, after all other implementations from contrib, you need to make it procedural.
As a contrib author, to be maximally compatible, you still need to follow this advice even if a lot of contrib has converted to OOP.

An alternative could be to have a dedicated event listener for the procedural hooks, with priority 0.
This way, OOP hooks can run before or after all procedural hooks.

Why should we care?
- Cost of upgrading major versions can cause projects to jump ship.
- I have seen more cases where order of hook implementations was important.
- A lot of contrib and custom modules will keep their procedural hooks for a long time.

(My other solution attempted to address this problem, but was arguably overengineered.)

Reverting the change means that the original set of people get the problem again. Which is an option, for sure, but it's not making the problem go away.

I would propose a more flexible solution:
- We choose one default sort order that will also be used for Drupal core. This should follow what existing tools already support, and what is common in existing 3rd party code.
- Contrib modules or website projects are allowed to use their own order. We can reference documentation how to configure this with Coder and phpcs.xml.

In the issue summary I don't see any argument why the proposed order (case sensitive) would be preferable to something else.
I also don't see a statistical analysis for existing code in and outside of Drupal.
(might be in the comments)
I only see the argument that having a convention is better than having no convention.

As for existing code, the following two regex searches can be used to search in a vendor directory:
(I did the search in PhpStorm with case sensitivity and multiline enabled)

use ((\w+\\)+\w+)[A-Z]\w+.*;
use \1[a-z]\w+.*;

use ((\w+\\)+\w+)[a-z]\w+.*;
use \1[A-Z]\w+.*;

The "\1" references a previously captured substring.

For all the code I found, it seemed that case insensitive sorting was used.
But even in a typical Drupal project these were just a few matches overall.

Drupal is an outlier because module names in namespaces are lowercase.
(I remember back in the days I argued for this, so that we see the literal module name in the namespace, not a captitalized form. I still think this was the right choice.)

catch → credited donquixote → .

so actually the patch in #2 is the way to go :)

On the other hand it is questionable why any logged-in user should see this message which is quite technical.

Actually a relevant part of the problem is that user.module is not loaded when adaptivetheme_preprocess_maintenance_page() is called.
So we could fix it with module_load_include('module', 'user').

Also, I expect that any software developer knows what case-sensitive ordering is so we don't need the last line. Correct me if I am wrong.

I would disagree.
A case-sensitive comparison is clear.
But a case-sensitive order, not so much. It could be derived from a character set (e.g. ASCII) where letters have numeric representations, but this is by no means clear. It could also be based on whatever specific php sort functions do.
E.g. for regular letters it could be "A, B, C, ..., a, b, c, ..." or it could be "A, a, B, b, C, c, ...".
For special letters like 'ä' it is less clear. Although I think we don't allow them in namespaces or module names, or people just avoid them and stick to a...z.

slevomat uses strcasecmp() and strcmp() depending on the setting.
This will result in "A, B, C, ..., a, b, c..." for regular chars.

Here are some experiments, https://3v4l.org/iUKZh

With my current version of PhpStorm, the auto sort with Ctrl+Alt+O (Optimize imports) puts the imports in the "wrong" order according to the new standard.
But I also see cases of the wrong order in Drupal core (maybe because I am not in latest version).

It seems the following happened:
- The 3.x branch was merged into the issue MR branch.
- The 3.x branch was rebased or reset to the tip of the issue MR branch.

Actually more likely it was just merged with fast-forward.

Hi
Thanks for merging,
but I think it would be better to reset (and force-push) to an older version to get rid of the faulty commits that caused this.
Let's talk on slack.

The commits from this MR made it into the 3.x branch in a bad way, bypassing the pipeline tests.
It seems the following happened:
- The 3.x branch was merged into the issue MR branch.
- The 3.x branch was rebased or reset to the tip of the issue MR branch.
- More minor changes were pushed to the MR branch.
- The minor changes were then squash-merged into the 3.x branch, resulting in https://git.drupalcode.org/project/l10n_client/-/commit/ff75bc814e058add...

Since this we have failing tests.

I think the correct way is to hard-reset the 3.x branch to before this issue, which is 3.0.0-alpha3.
I don't remember if drupalcode.org allows us to hard reset a branch (that is, force push).

Another import went missing in the same commit, for PoItem.
Adding it in the MR.

I could add tests, but this functionality is still quite fluid, so we should avoid unnecessary effort.

Hello.
I was able to reproduce with latest version of this module both with Drupal 10 and Drupal 11.

For now the way to resolve it is to manually remove the field before uninstall.
Fingers crossed :)

We need to think about what would be the optimal solution to implement in this module.

One thing to consider is that removing the field will cause data loss, because all the tokens per user are lost.
This is why there is some merit in letting the user manually delete the field.

On the other hand, this deletion also would need to happen in a deployment. So it would need a custom update hook in the project to delete the field, which would be a burden on the site builder.

donquixote → created an issue.

Personally, I’ve been using “Constructs a new class instance.” as the first line, which still follows the “start with third-person verb” rule

even though it’s a bit longer, it’s in line with the almost universal format for method doc comments and once you read “Constructs” you already know what’s going on.

For me, magic methods and especially the constructor are special because you (typically) don't call them directly.
(For the constructor, an exception would be calling the parent class constructor.)

The "start with third-person verb" gives you a promise what you can expect by calling the method.
It would be justifiable if we decide to not do this on some or all magic methods.

donquixote → created an issue.

I would not put too much emphasis on the performance argument.
The medium article from #20 looks interesting, but I have not looked into it enough to assess it.
Certainly it's good to understand what we are optimizing for.

Currently we live in a world where most functions are in the global namespace.
This applies to native php functions and to functions from Drupal modules.
Many packages don't want to add functions, they only want to add classes.
(there have been interesting discussions in the php internals list about static methods vs functions)

If more packages and modules embrace and provide namespaced functions, the potential for ambiguity becomes bigger.

One thing here is technical ambiguity, where actually the wrong function is called.
The other thing is confusion when reading code, because you don't immediately see if a function is in the global namespace or not.

I was long convinced that backslash is the way to go, but maybe we should also consider imports, and check how projects outside of Drupal do it.

had no idea this was added.

Yeah this is quite new and not part of any stable release yet if I saw correctly.
I only found it because I really wanted to have machine names.

Perhaps core should provide better detectability of this change, before anything is committed to ds or field_group.
Until then, people can use this patch if it is useful to them.

For now I am using the class 'machine-name' for feature detection.
Ideally, core should provide a better way for contrib modules to detect if the machine name column is present.

donquixote → created an issue.

This patch is a first proof of concept, definitely not to be committed as-is.
It lacks the conditionality, and it lacks tests.

It is ok for people who want to patch both core and field_group to get the machine name column.

The change seems simple, but the conditionality is more tricky.

We should have some kind of feature detection support in core.

It could also be nice to have some kind of feature detection possibility, so that a module like field_group can support different versions.
Currently it could check if any field row has a cell key 'machine_name', but this is not 100% reliable if there are no fields for some reason.

donquixote → created an issue.

I like this change. Thanks everybody!

Just some notes:

• The columns gets even more tight when I click "Show row weights".
  I like to do this to manually set row weights to reduce noise in a git commit.
  But it is worth it imo.
• This needs work in field_group and possibly other contrib modules. Currently it looks broken with field_group enabled.
  Again, that's ok with me.
  But it deserves a note in the "API changes".

From a usability perspective:
In all the projects I ever worked on, the site builder who manages displays with field_ui is the same person who will then run "drush config:export" and commit the changes in git.
There is no point in hiding machine names from this person.

donquixote → created an issue.

I just noticed a workaround:

• When you save the site config that has the urls for front, 403 and 404, even if you specify an alias, it stores the `/node/123` url.
• However, if you then manually edit the config files, put the alias instead of `node/123`, and import the config, that actually works.

And btw, if I run "migrate:import --update" with a node or term migration with this patch, it does work, and it does update the entity uuid to the correct value.

We probably do need something else for revision uuids.
But in a first version we can live without them.
Term uuids and some content uuids are important if they are referenced from elsewhere, esp in config.
Revision uuids are quite unlikely to be referenced in config. The main reason to support them would be a sense of technical completeness.

Here is a patch that adds to the #7 patch.
This only covers the first part of the problem, exposing uuid for nodes.

Actually it might be a good idea to treat this as two separate issues:
- Add uuid for node source plugin, if uuid module was installed in D7.
- Follow-up to add uuid mapping to the default migrations.

I like patch #7 as an incremental improvement.
For it to work correctly, we also need to add 'uuid' in the ->fields() method, with the same condition.

As for adding the "uuid: uuid" mapping to migrations:
One option is to conditionally add this mapping. So it would not be present in the default yml files.

If we always want to add the "uuid: uuid" mapping, then we need to auto-fill this if the uuid module was not installed in D7.

The usual random uuid is not great, it will produce a different value each time the migration is run.
This causes issues e.g. with views that reference taxonomy terms by uuid.

I can see two options to generate "stable" uuids, if uuid module was not enabled in D7:
- Store the generated uuids in a yml file somewhere, which would be committed to code. I don't like this, because this file would have to scale with the amount of content.
- Generate fake uuids that are actually hashes based on entity type, entity id, and a fixed salt defined somewhere. E.g. create a sha256, get a substring, insert the dashes for uuid format, done.

Maybe all of this would have to be opt-in, to not mess with exiting migrations.

And yes, test coverage.

donquixote → created an issue.

I added more test classes..

benjifisher → credited donquixote → .

In my local I get these failures with the FAIL branch:

Time: 00:37.938, Memory: 4.00 MB

There were 4 failures:

1) Drupal\Tests\symfony_mailer\Functional\LegacyEmailTest::testSendLegacyEmail
Failed asserting that null is not null.

/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:122
/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:56
/var/www/html/tests/modules/symfony_mailer_test/src/MailerTestTrait.php:46
/var/www/html/tests/src/Functional/LegacyEmailTest.php:43
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:729

2) Drupal\Tests\symfony_mailer\Functional\LegacyEmailTest::testSendLegacyEmailWithTheme
Failed asserting that null is not null.

/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:122
/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:56
/var/www/html/tests/modules/symfony_mailer_test/src/MailerTestTrait.php:46
/var/www/html/tests/src/Functional/LegacyEmailTest.php:81
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:729

3) Drupal\Tests\symfony_mailer\Functional\OverrideTest::testUpdate
Failed asserting that null is not null.

/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:122
/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:56
/var/www/html/tests/modules/symfony_mailer_test/src/MailerTestTrait.php:46
/var/www/html/tests/src/Functional/OverrideTest.php:127
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:729

4) Drupal\Tests\symfony_mailer\Functional\TestEmailTest::testTest
Failed asserting that null is not null.

/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:122
/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:56
/var/www/html/tests/modules/symfony_mailer_test/src/MailerTestTrait.php:46
/var/www/html/tests/src/Functional/TestEmailTest.php:27
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:729

FAILURES!
Tests: 26, Assertions: 274, Failures: 4.

With the 1.x branch I get 1 failure:

Time: 00:47.724, Memory: 4.00 MB

There was 1 failure:

1) Drupal\Tests\symfony_mailer\Functional\OverrideTest::testUpdate
Failed asserting that null is not null.

/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:122
/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:56
/var/www/html/tests/modules/symfony_mailer_test/src/MailerTestTrait.php:46
/var/www/html/tests/src/Functional/OverrideTest.php:127
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:729

FAILURES!
Tests: 26, Assertions: 312, Failures: 1.

With the fix branch, all tests pass.

This is interesting.
The "FAIL" merge request does not actually fail the phpunit test.
But in my local env (with ddev) I do get test failures, even without the usleep().

One thing I suspect is that usleep()/sleep() does not really work in the pipeline.

Also, the current solution with using state assumes that all emails are sent during a single request.
If there are multiple requests that happen before the test can collect the emails, e.g. as a batch process, and each of them is sending emails, then there will be a problem in the __construct() which expects the state collected mails to be empty.

A database table with one row per mail and auto increment would solve this.
There can be a counter in the test trait that contains the last read auto increment id.
In the ::setUp() method the table can be truncated, and the auto increment reset, OR the internal counter variable/property can be set to the current auto increment counter from the db.

We also want this to work with ExistingSite tests, where we cannot rely on the module being freshly installed each time.

Actually...
this is not a ::__destruct() method, but a ::destruct(), which is called from $kernel->terminate().

We see this in index.php:

$response->send();

$kernel->terminate($request, $response);

So this explains the order :)

donquixote → created an issue.

donquixote → created an issue.

Hello,
it makes sense to remove compiler passes that are purely for optimization, and that don't change behavior.

However, the RemoveUnusedDefinitionsPass is not designed purely for optimization.

In a symfony project, the following use case is typical:
- Scan a directory and register every class as a private service. This will include classes that absolutely cannot be used as services.
- Process all the services with autowire, autoconfigure etc. This will also populate the error list for broken services.
- Remove unused private services. This will usually get rid of the broken services that were added earlier.

This means RemoveUnusedDefinitionsPass is essential if we have any mechanism that adds random classes as private services.
If we don't have this mechanism, then we never need it: Not during the installer, but also not really later.

I mention all this because I am currently experimenting with service discovery.

We simply need to drop the second commit :)

Or we could keep the test case from the second commit, but change the expected behavior.

Mostly accept the PR, except lets just stop at isInstantiable, and just ignore abstracts. (tests and fixtures need updates as a result)

We simply need to drop the second commit :)

But I want to be clear: Currently the abstract class create() does work.
So if we don't have the second commit, we lose functionality, and theoretically break BC.

The example in the test is quite obscure. Perhaps somebody has a more valid use case.
Of course such cases would be easy to fix.

On the other hand, I can also imagine cases where this behavior would be clearly unintentional.

I think it is good practice that a class with hook methods shall be either final or abstract, to clarify intent.
Otherwise, if a hook method is inherited from a non-abstract base class, it is not clear if the developer intended this to be registered as a hook or not.
But we can't really enforce this now.

The test cases cover some "hidden" features which already work today, but the support of which may not have been by design.