Hello,

In the case of an external usage of the diff module, see #3088227: Use diff in another context, a revision may not have an author and so it provokes a fatal error.

I will upload a patch for that.

CommentFileSizeAuthor
#6 diff-revision_no_author-3088274-6.patch1.28 KBzarpele
#5 diff-revision_no_author-3088274-5.patch1.32 KBzarpele
#2 diff-revision_no_author-3088274-2.patch1.64 KBgrimreaper

Issue fork diff-3088274

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

Grimreaper created an issue. See original summary.

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Assigned: grimreaper » Unassigned
Status: Active » Needs review
StatusFileSize
new diff-revision_no_author-3088274-2.patch1.64 KB

Here is the patch.

Thanks for the review.

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Related issues: +#3088227: Use diff in another context
zarpele’s picture

zarpele commented

I tested it and it works as expected.

Thanks for the patch

zarpele’s picture

zarpele commented
StatusFileSize
new diff-revision_no_author-3088274-5.patch1.32 KB

Well, after a deeper test, the patch doesn't work when the user removed is the last editor on the revision list page.

Apparently Drupal keeps the revision user id even when a user was deleted.

 $user_id = $revision->getRevisionUserId();

My proposal/patch is to check if the object is available before trying to display the name.

if (!is_null($revision->getRevisionUser())) {

Note: Patch #5 fails. I attached the correct one below (#6)

zarpele’s picture

zarpele commented
StatusFileSize
new diff-revision_no_author-3088274-6.patch1.28 KB
plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
Status: Needs review » Reviewed & tested by the community

Patch still applies and we have been using it in production sites for a while.

Would be great to have it merged.

Marking as RTBC.

acbramley’s picture

acbramley commented
Status: Reviewed & tested by the community » Needs work

Thanks for the work on this one. I've recently taken up maintainership of this project and am looking through the RTBC issues.

This fix looks good.

To get this in, I'll need an MR rebased against the latest 8.x-1.x code with tests added.

Thanks!

acbramley’s picture

acbramley commented
Category: Feature request » Bug report
Issue tags: +Needs tests

silvi.addweb made their first commit to this issue’s fork.

silvi.addweb opened merge request !59

silvi.addweb’s picture

silvi.addweb commented
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs tests

Hello, I have raised MR for the same.

acbramley’s picture

acbramley commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Don't remove the tag if tests haven't been added please.

joshua1234511 made their first commit to this issue’s fork.

joshua1234511’s picture

joshua1234511
Primary language English
Location Goa
commented
Status: Needs work » Needs review
Issue tags: -Needs tests

Fixed failing MR.
Added the Required Test.

acbramley’s picture

acbramley commented
Status: Needs review » Needs work

Tests are failing.

acbramley’s picture

acbramley commented
Version: 8.x-1.x-dev » 2.x-dev

acbramley closed merge request !59

acbramley changed the visibility of the branch 8.x-1.x to hidden.

acbramley changed the visibility of the branch 3088274-prevent-fatal-error to hidden.

acbramley opened merge request !133

acbramley’s picture

acbramley commented
Status: Needs work » Needs review

Thanks for everyone's contributions to this, however i have taken the solution in a different direction.

The type, title and url keys on this array were completely unnecessary, template_preprocess_username doesn't use any of them. We can simply remove those lines to allow falling back to an Anonymous user display when the revision user is NULL. Also added much simpler test coverage using entity_test

acbramley’s picture

acbramley commented

  • acbramley committed 50443f43 on 2.x 
    Issue #3088274 by zarpele, acbramley, joshua1234511, grimreaper: Prevent...
acbramley’s picture

acbramley commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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