Problem/Motivation

PhpStorm with PHP Inspections reports an error on Core profiles standard and demo_umami

Steps to reproduce

Open PhpStorm with PHP Inspections and let it index core.
protected visibility

Proposed resolution

Fix it. Please see attached patch.

Remaining tasks

Review

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

...

CommentFileSizeAuthor
#4 fix-core-profiles-member-protected-visibility-3260568-4.patch1.13 KBbserem
#2 fix-core-profiles-member-protected-visibility-3260568-1.patch1.13 KBbserem
core_profiles_protected_visibility.png115.34 KBbserem
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bserem created an issue. See original summary.

bserem’s picture

bserem CreditAttribution: bserem at Point Blank for DXC.technology commented
FileSize
fix-core-profiles-member-protected-visibility-3260568-1.patch1.13 KB
bserem’s picture

bserem CreditAttribution: bserem at Point Blank for DXC.technology commented
Status: Active » Needs review
bserem’s picture

bserem CreditAttribution: bserem at Point Blank for DXC.technology commented
Status: Needs review » Active
FileSize
fix-core-profiles-member-protected-visibility-3260568-4.patch1.13 KB
bserem’s picture

bserem CreditAttribution: bserem at Point Blank for DXC.technology commented
Status: Active » Needs review
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Category: Bug report » Task
Priority: Normal » Minor
Issue tags: +Bug Smash Initiative

Nice cleanup - looks good to me

Are there any other calls like this in core (outside umami)

If so we should fix them all

cilefen’s picture

cilefen CreditAttribution: cilefen as a volunteer commented

I sure don't mind commonsense fixes like this, but, won't the PHPStan project find these and more?

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented

I'm with @cilefen.

I strongly suspect that Migration tests do this for various properties.

bserem’s picture

bserem CreditAttribution: bserem at Point Blank for DXC.technology commented

@larowlan PhpStorm doesn't highlight anything else for me, only standard and umami

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented

I think PhpStorm is actually wrong here. I get the same warning, but then if I press Ctrl+B on the highlighted $user->roles property, it takes me to \Drupal\filter\Entity\FilterFormat::$roles which is irrelevant.

$user->roles is a dynamically defined magic entity property, and PhpStorm doesn't understand these. I assume it's somehow figuring out from docblocks that User::load() returns an EntityInterface, and FilterFormat is the only EntityInterface in core that has a $roles property, so it wrongly assumes it must be that one.

If PhpStorm was right, I don't think standard_install() would work - the administrator role would never be assigned to user 1 - but tests show this is working as intended.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented

@longwave you are right! Wow, I never would have thought of that. But when I temporarily change the name of the $roles property of FilterFormat the "error" disappears.

Yes, the current code is not broken, but the patch makes it more readable/self-documenting and it also removes a common (at least for me!) annoyance for people using PhpStorm. So even if PhpStorm is being a bit weird in this situation for me it's still a win-win to commit this patch. Not marking RTBC as I don't want to undermine any further discussion or reservations, but ++.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs review » Reviewed & tested by the community

Agree we should use the API where possible to set a good example, and conveniently this also fixes the PhpStorm issue, so I'm going to mark this RTBC anyway.

Kartagis’s picture

Kartagis
Primary language Turkish
Location Istanbul
CreditAttribution: Kartagis as a volunteer commented

I get the same "error" on $form_state->input and Cmd+B takes me to core/lib/Drupal/Core/Form/FormState.php, which says protected $input;. I must add that this is the standard profile.

bserem’s picture

bserem CreditAttribution: bserem at Point Blank for DXC.technology commented

@kartagis I can't find any occurences of $form_state->input in the standard profile. May I suggest to create a followup issue and provide more information about it there?

Kartagis’s picture

Kartagis
Primary language Turkish
Location Istanbul
CreditAttribution: Kartagis as a volunteer commented

@bserem yeah, sorry. I should've mentioned that $form_state->input is in a custom module.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 4: fix-core-profiles-member-protected-visibility-3260568-4.patch, failed testing. View results

bserem’s picture

bserem CreditAttribution: bserem at Point Blank for DXC.technology commented
Status: Needs work » Reviewed & tested by the community

At some point automated tests failed for D9 for reasons not related to this issue. I asked for tests to run again and they are green again.

Putting this back to RTBC

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed 056040934b to 10.0.x and 123f259d82 to 9.4.x. Thanks!

  • alexpott committed 0560409 on 10.0.x 
    Issue #3260568 by bserem, longwave: Fix "Member has protected visibility...

  • alexpott committed 123f259 on 9.4.x 
    Issue #3260568 by bserem, longwave: Fix "Member has protected visibility...

Status: Fixed » Closed (fixed)

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