Invalid code in DiscussionBestAnswerPage, condition always true

[clarkwinkelmann]

I was looking at the source code after seeing something odd in the screenshot shared by a user on the forum https://discuss.flarum.org/d/18316-flarum-seo/604

flarum-seo/src/Page/DiscussionBestAnswerPage.php

Line 153 in da3cb93

"name" => $discussion->user() ? $discussion->user()->first()->getDisplayNameAttribute() : null

$discussion->user() ? $discussion->user()->first()->getDisplayNameAttribute() : null is valid code but doesn't do what is intended. $discussion->user() will always return true because it returns the Eloquent relationship definition object.

There's actually a similar bit of code a few lines below that uses the proper approach:

flarum-seo/src/Page/DiscussionBestAnswerPage.php

Line 189 in da3cb93

"name" => $post->user ? $post->user->display_name : null

In the same file I see some more strange code but it shouldn't be causing any logic error by itself:

flarum-seo/src/Page/DiscussionBestAnswerPage.php

Lines 159 to 160 in da3cb93

	if ($discussion->tags()->count() >= 1) {
	$tags = $discussion->tags()->get()->all();

causes 2 SQL requests to be made, and I don't know where this code is called from, but the relationship might already be loaded, resulting in 3 total requests for this. Using $discussion->tags directly would re-use the already cached results. If it's known that they are not loaded, then there can be an argument for 2 requests if you want to minimize the memory and transfer size but I'm sure response time would still be better with just one request. In any case ->tags()->get() can be replaced with ->tags.

Code can also be inlined ->generateSchemaBreadcrumb($discussion->tags->map(function() {})->all()) 🙉