New constants in \Drupal\Core\Theme\Registry are private but they are accessed with static::

Created the mr so I could review.

Is it better to just remove private?

Just noting @larowlan wanted them private originally, just curious if we should re evaluate here or if self is enough.

Imo leave them as private - I've added a PHPStan rule that covers this situation. It's just a mistake to access private constants with static.

As the methods that use the constants are protected shouldn't the constants also be protected? Or are we saying you shouldn't actually be overriding those methods?

https://git.drupalcode.org/project/drupal/-/merge_requests/11796#note_50...

This is where it was discussed.

That failure might be real, I have rerun it 3 or 4 times.

The failure has nothing to do with this change - it is one of the frontend changes from today.

🐛 Fix AssetAggregationAcrossPagesTest Active is the fix for the testing issue on 11.x

Looks like the idea behind making them private was because they might be temporary and be removed later, I think the MR here is fine for a quick fix, we can re-evaluate the constants themselves later.

I saw remarks elsewhere that some other recent commit might have added instability to the functional JS tests, so I queued it again.

The one that Alex posted is the test that was failing, this has not been rebased so it won't pass tests.

Also do we have a link to the other constants where they got added? I am only familiar with the registry ones but there are other constants in the mr.

The failure is to do with asset aggregation in Umami:

 Time: 03:16.817, Memory: 10.00 MB
452
    
453
    Asset Aggregation Across Pages (Drupal\Tests\demo_umami\FunctionalJavascript\AssetAggregationAcrossPages)
454
     ✔ Front and recipes pages
455
     ✔ Front and recipes pages authenticated
456
     ✘ Front and recipes pages editor
457
       ┐
458
       ├ Asserting ScriptBytes 335003 is greater or equal to 335137 and is smaller or equal to 336137
459
       ├ Failed asserting that 335003 ( is equal to 335137 or is greater than 335137 ) and ( is equal to 336137 or is less than 336137 ).
460
  
466
    FAILURES!
467
    Tests: 3, Assertions: 29, Failures: 1, PHPUnit Deprecations: 8.

So if that was the exact fail as before or after, we might worry. But I just checked and requeued the dailies, and they're also full of functional JS fails.

Sorry, crossposted with the above.

I think this is fine as a hotfix; we can file followups if we want to discuss further or change other constants.

Saving issue credits; hopefully the rebased pipeline will be done by then.

For some reason now it's not triggering the later stages of the pipeline.

OK same issue as with the automated pipeline:

???

(and yes I am logged in)

I see that too, I am also logged in. Might be worth asking in infra.

I created a new branch from this one too trigger a fresh pipeline to see if that works.

Unfortunately I had typos in the title lol.

I did leave a message about it in #gitlab, yeah.

Committed the results from the helpful tesr-pupeline branch 🤣 to 11.x.

It doesn't cherry-pick cleanly to 11.2.x in core/lib/Drupal/Core/Theme/Registry.php, but we should make a separate version without the PHPStan rule addition anyway.

I did it from my phone lol.

Pipeline weirdness might be fixed by giving yourself push access to the branch, I think I've had the same thing happen.

alexpott → changed the visibility of the branch 3531412-tesr-pupeline to hidden.

alexpott → changed the visibility of the branch 3531412-new-constants-in to hidden.

I've cherry-picked the 11.x commit to the 11.2.x and resolved conflicts and backed out the PHPStan change.

I ran vendor/bin/phpstan analyse -c core/phpstan.neon.dist core/lib/Drupal/Core/Theme/Registry.php --level=2 and confirmed that the constant issue is fixed on 11.2.x too.

Looks like it was these lines that were added in 11.x and prevented the cherry-pick, so it's a good thing we added the rule to HEAD at least:

-      $cache[static::GLOBAL_PREPROCESS] = $this->collectModulePreprocess($cache, 'preprocess');
+      $cache[self::GLOBAL_PREPROCESS] = $this->collectModulePreprocess($cache, 'preprocess');

We should possibly backport the rule itself to 10.6.x as well.

Also checked the file as per #35. I probably should have run a full scan, but this is, after all, a hotfix. That is also why I am not requiring followups before commit. Committed to 11.2.x. Thanks!

Setting NW for the followups.

FWIW now we have the PHPStan rule in place, I think private constants are fine. In this case the constant is for an implementation detail of the code in \Drupal\Core\Theme\Registry - using a private constant means that anything that extends it should not rely on this implementation detail - which feels like fine architectural decision to me. I think we should only open a follow-up if we have an architectural reason to allow classes that extend \Drupal\Core\Theme\Registry access to the constant. For Twig UI this is not necessary at all. Somewhat sadly the 8.x-3.x version of Bootstrap hit this problem a couple of weeks ago but fixed it incorrectly in the Bootstrap theme rather than opening a core issue - see 🐛 Undefined constant Drupal\bootstrap\Plugin\Alter\ThemeRegistry::PREPROCESS_INVOKES in Drupal\Core\Theme\Registry->postProcessExtension() Active - I will open an issue against Bootstrap to revert that change once we have released the hotfix.

Ah the bootstrap fix is yet to be merged. Another reason to get this fix released ASAP as we now have 2 confirmed cases in contrib and Bootstrap 8.x-3.x has about 50,000 users → .

My two cents is if we are worried about people resulting in it we should have just used the array value.

If we want them to be constants for consistency then they should be marked internal.

Just because we don't know why someone would want to use it doesn't mean we should preclude it.

This issue is an example of why, if they had not been private I don't think this issue would have existed in the first place right?

@nicxvan if they were not private yeah no issue... but the issue is not caused by the code in modules trying to use the constant. It is caused the late static binding... see https://3v4l.org/MDU2P

I think we now have static analysis checking then it is fine to have private constants but before I'd agree that it is very easy to make a mistake.