Core: [Mentions] ErrorException: Trying to get property 'display_name' of non-object

[sentry-io[bot]]

Mentions extension cannot generate display_name for user that has been deleted. This happens when trying to start mentioning inside a discussion where the deleted user was active (I assume).

Sentry Issue: DISCUSS-2Z

ErrorException: Trying to get property 'display_name' of non-object
  File "/vendor/flarum/mentions/src/ConfigureMentions.php", line 107, in FoF\Sentry\SentryServiceProvider::handleError
    $tag->setAttribute('displayname', $post->user->display_name);
  File "/vendor/flarum/mentions/src/ConfigureMentions.php", line 107, in Flarum\Mentions\ConfigureMentions::addPostId
    $tag->setAttribute('displayname', $post->user->display_name);
  File "/vendor/s9e/text-formatter/src/Parser/FilterProcessing.php", line 101, in s9e\TextFormatter\Parser\FilterProcessing::executeFilter
    return \call_user_func_array($filter['callback'], $args);
  File "/vendor/s9e/text-formatter/src/Parser/FilterProcessing.php", line 69, in s9e\TextFormatter\Parser\FilterProcessing::filterTag
    self::executeFilter($filter, $vars);
  File "/vendor/s9e/text-formatter/src/Parser.php", line 656, in s9e\TextFormatter\Parser::processStartTag
    FilterProcessing::filterTag($tag, $this, $this->tagsConfig, $this->openTags);
...
(56 additional frame(s) were not displayed)

[franzliedke]

Based on the exception, it happened when editing. I cannot reproduce it (yet), though.

@datitisev Any memory of what happened there?

[datitisev]

@franzliedke It's when you try to mention a deleted user / deleted user's post. It initially looks fine because the JS processing can handle the [deleted], but when then php processes it it errors

[franzliedke]

Oh, duh. I didn't press "Save" and was waiting for client-side problems. :face_with_head_bandage:

[oddjob79]

Hello,
I was looking at your project and would like to begin contributing, if you are all ok with it?
I have a dev environment setup locally and think I have found the source of the issue. Do you mind if I work on it?

I do have a question though. I'm not really sure how to manage the branching. It looks like the only change will be to the mentions repository. I have this cloned locally, so do I just create a branch off the master branch of the mentions repo and work on that, or should I pull down the dk/permissions branch and create a branch off that one?

Thanks in advance,
Rob

[franzliedke]

@oddjob79 Welcome and thanks for contributing!!!

Yes, indeed, you can branch off master in the mentions repository. When you're done, commit and push the branch to a personal fork of the mentions repository, then GitHub will propose opening a merge request. :smiley:

[oddjob79]

Fantastic! Thank you @franzliedke.
I probably won't be able to work on it again until Monday, but will keep you informed if I have any questions or issues.

[oddjob79]

Hi @franzliedke ,
Sorry for the delay in getting back to you. I believe I have a fix prepared for this, but have been really struggling to get a unit test working which will demonstrate this.

I was able to write something which would run against an existing implementation of flarum, but have been struggling to utilize the api to build a self-contained test which would create a user, post from the user, delete the user, then create a new post which mentions the user. I believe that is what would be necessary to properly test this out using phpunit?

Before I spend any more time looking to do this, I just wanted to check in with you to make sure this was necessary, and if you had any advice / instruction on utilizing the testing scripts I see in the core package. Currently my test script returns a RouteNotFoundException against api/users, despite the fact I can hit the url with curl without any problem.

Thanks in advance for any direction.

[franzliedke]

@oddjob79 Hmm, to be honest, we don't yet have a good story for testing features in (bundled) extensions. So don't worry about that for now.

As for the tests: have you run the composer test:setup command? Or rather, do the other integration tests (composer test:integration) succeed?

[oddjob79]

@franzliedke Thanks for your reply. I took your advice and submitted the pull request without further diving into the realms of phpunit testing. Hopefully that is now available to you for review. Please let me know if it requires any amendments or if the submission process was not quite right.

With regards the integration tests, thanks for the tip! This was helpful in terms of running the existing tests. I had been attempting to write additional test scripts utilizing existing classes and running them via vendor/bin/phpunit.

Unfortunately, I got the same results with regards the RouteNotFoundException. I'm sure you have better things to do with your life than troubleshoot this for me, but I have attached the output I am receiving when running the composer test:integrate command, in case you have any quick suggestions. I should mention I upgraded phpunit to version 8^, in case this might be having an impact?

Thanks again, and I'll wait to hear from you regarding the pull request.

phpunit.txt

[oddjob79]

@franzliedke I re-opened the pull request. I think this is now ready for your review and hopefully to merge it in. Please let me know if there's anything else I need to do to complete this.

[franzliedke]

Hmm... now that this is fixed, the JS preview shows a nice [deleted] label, but once the post is saved (at least this does not break anymore), the raw mention tag (e.g. @3#11) is displayed instead.

Do we want to do anything about this?