Problem/Motivation

This is a quirky issue relating to EntityStorageInterface::loadRevision being deprecated.

PHPStan is caching interface methods from left to right.

@Kingdutch had some information on the internals for how PHP handles this: https://drupal.slack.com/archives/C033S2JUMLJ/p1692251803456129?thread_t...

Kingdutch on August 17th at 7:56:43 AM
This sounds very similar to https://github.com/phpstan/phpstan/issues/8437 that I filed for https://www.drupal.org/project/drupal/issues/3324733 which was slightly different
(We have more of these hidden in Drupal core)
Usually these things are not PHPStan issues but PHP quirks :sweat_smile:
PHP does not document its interface inheritance order, but I can find https://www.php.net/manual/en/language.oop5.interfaces.php#125893 which links to 3 bug reports that suggest that the evaluation happens from right to left?
If we follow the right to left logic, I think things suddenly make sense and that's exactly why flipping them fixes it (because RevisionableStorageInterface which has the most specific method, comes last)

I don't think it really matters. Drupal core just extends EntityStorageInterface a lot.

Steps to reproduce

Scan code with PHPStan and deprecation rules package. Get warnings.

Proposed resolution

Stop extending `EntityStorageInterface` if another interface is extending it.

Rearrange extends for TranslatableRevisionableStorageInterface

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3383215

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

mglaman created an issue. See original summary.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman as a volunteer commented

Also. R before T so it fixes alphabetical sorting 🤷‍♂️. May not be testable until #3263109: Use PHPStan for deprecation checking

mglaman opened merge request !4637

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman as a volunteer commented

Actually, it looks like I can fix this in phpstan-drupal after all. So it can bridge the gap without committing this https://github.com/mglaman/phpstan-drupal/pull/596

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman as a volunteer commented
Status: Active » Needs review

Nevermind, I forgot it asserts an incorrect error in the test:

        $errors = [];
        if (version_compare(\Drupal::VERSION, '10.1', '>=')) {
            $errors[] = [
                'Call to deprecated method loadRevision() of class Drupal\Core\Entity\EntityStorageInterface:
in drupal:10.1.0 and is removed from drupal:11.0.0. Use
\Drupal\Core\Entity\RevisionableStorageInterface::loadRevision instead.',
                12
            ];
        }

This is from:

$genericContentEntityStorage = \Drupal::entityTypeManager()->getStorage('foo');
assert($genericContentEntityStorage instanceof ContentEntityStorageInterface);
$genericContentEntityStorage->loadRevision(1);

So the generic content entity storage interface doesn't work due to ordering of extends.

bbrala’s picture

bbrala
Location Netherlands
CreditAttribution: bbrala as a volunteer and at SWIS commented
Status: Needs review » Reviewed & tested by the community

We talked about this on Slack and there is no real possible problem that this could bring. Tests are also green. Setting rtbc

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Needs work

This is going to need a re-roll after #3383279: Bump mglaman/phpstan-drupal to latest to make daily "updated deps" QA run pass again

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman as a volunteer commented

Rebased the MR

spokje’s picture

spokje
Primary language Dutch
CreditAttribution: spokje as a volunteer commented
Parent issue: » #3383289: [META] Fix the PHPStan baseline suppressions added for mglaman/phpstan-drupal:1.2.0
spokje’s picture

spokje
Primary language Dutch
CreditAttribution: spokje as a volunteer commented
Issue tags: +PHPStan-1
spokje’s picture

spokje
Primary language Dutch
CreditAttribution: spokje as a volunteer commented
Status: Needs work » Needs review
bbrala’s picture

bbrala
Location Netherlands
CreditAttribution: bbrala as a volunteer and at SWIS commented
Status: Needs review » Reviewed & tested by the community

Baseline updated, rebated, looking good.

spokje’s picture

spokje
Primary language Dutch
CreditAttribution: spokje as a volunteer commented

Come to think of it: There might very well be a Coder follow-up in here to force the implements to be in alphabetical order?

  • catch committed 70e2c380 on 11.x 
    Issue #3383215 by mglaman, Spokje, bbrala: Rearrange interfaces...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 11.x, thanks!

Alphabetical implements seems worth opening a follow-up issue for if that'll help the phpstan baseline, would also match how we alphabetise use statements.

spokje’s picture

spokje
Primary language Dutch
CreditAttribution: spokje as a volunteer commented

Opened Coder issue #3383525: Check alphabetical ordering for "implements"

bbrala’s picture

bbrala
Location Netherlands
CreditAttribution: bbrala as a volunteer and at SWIS commented

Its not about alphabetical but about load order, so please don't force alphabetical :)

spokje’s picture

spokje
Primary language Dutch
CreditAttribution: spokje as a volunteer commented

Also. R before T so it fixes alphabetical sorting 🤷‍♂️

Its not about alphabetical but about load order, so please don't force alphabetical :)

Confused now, but closed Coder issue anyway.

bbrala’s picture

bbrala
Location Netherlands
CreditAttribution: bbrala as a volunteer and at SWIS commented

It's a cooincedence it's alphabetical, there is a load order to interfaces in php. Which is logica but very edge casey. We might run into this in the future. See the Slack thread.

kingdutch’s picture

kingdutch
he/him
Primary language English
CreditAttribution: kingdutch at Open Social commented
Issue summary: View changes

Just copying in my Slack message in case people don't want to (or can no longer) go there.

The TL;DR is: "Although undocumented, PHP, consistently and by design, evaluates interfaces from right to left"

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

mxr576’s picture

mxr576
Primary language Hungarian
Location Hungary
CreditAttribution: mxr576 as a volunteer and at Pronovix commented

Can this be also cherry-picked back to 10.1.x?

https://github.com/mglaman/phpstan-drupal/issues/621