Problem/Motivation

\Drupal\Core\Entity\RevisionLogInterface is incorrectly typed and causes static analysis tools like PHPStan to trip up.

Steps to reproduce

Run PHPStan on max where code calls \Drupal\Core\Entity\EntityTypeInterface::getRevisionUser/getRevisionUserId/getRevisionLogMessage. PHPStan will assume the returned variable is non empty. However returned values may not actually be set.

Proposed resolution

Change typehint, adding null values.

Remaining tasks

Implement

CommentFileSizeAuthor
#5 3309157-5.patch1.26 KBchaubeyji

Issue fork drupal-3309157

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

dpi created an issue. See original summary.

dpi opened merge request !2754

dpi’s picture

dpi
Location Perth, Australia
commented
Assigned: dpi » Unassigned
Status: Active » Needs review
dpi’s picture

dpi
Location Perth, Australia
commented
Title: RevisionLogInterface is typehinted as possibly returning entity/ids, but cannot guarantee set/existing values » RevisionLogInterface is typehinted as always returning entity/ids, but cannot guarantee set/existing values
chaubeyji’s picture

chaubeyji commented
StatusFileSize
new 3309157-5.patch1.26 KB
joachim’s picture

joachim commented
Status: Needs review » Reviewed & tested by the community

LGTM

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Needs review

Can you elaborate on the scenarios where the user ID or message are null

The entity one can occur when the user has been deleted.

dpi’s picture

dpi
Location Perth, Australia
commented

I was able to reproduce NULL log message errors in current core with this codeset: MR / Test results, primarily failing in revision UI tests and Migrate tests. However the user ID as null trigger didnt seem to error.

Pushed new tests which triggered exceptions with NULL for both user ID and log message. I anticipate this kind of issue to only arise when creating revisions programmatically.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

However the user ID as null trigger didnt seem to error.

The MR still has the type-hint change for that - should we roll that change back?

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

Discussed with @dpi who pointed out that the tests demonstrate the issue with user ID, so ignore #9

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Hiding the files to avoid confusion.

MR has no open threads and comments look good.

Not sure what else to verify.

  • catch committed 56923991 on 10.1.x 
    Issue #3309157 by dpi, larowlan: RevisionLogInterface is typehinted as...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Removing credit for the patch in #5, it duplicates the MR and doesn't include the new test coverage.

This looks good to me, tests that confirm the existing behaviour, and interface docs changes (which doesn't change the API as such just document the existing one properly).

Committed/pushed to 10.1.x, thanks!

Status: Fixed » Closed (fixed)

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

quietone’s picture

quietone commented
Issue tags: -Quickfix +Quick fix