Add `wp_get_update_php_annotation()` to return the Update PHP annotation

[afragen]

This function needs to return and not echo a response for #44350. Sorry my multitasking is clearly suffering.

wp_update_php_annotation() introduced in r44627

Ping @flixos

[flixos90]

That makes sense. However, I would suggest introducing a new wp_get_update_php_annotation() and keeping wp_update_php_annotation() as is. Many core functions have a variant that gets the output and another one that directly outputs it. wp_update_php_annotation() could just call echo wp_get_update_php_annotation().

This way we don't need to modify all the existing function calls, and we keep this a separate change (since it's only required for our 5.2 work, not anything scheduled for 5.1).

[afragen]

Replying to flixos90:

That makes sense. However, I would suggest introducing a new wp_get_update_php_annotation() and keeping wp_update_php_annotation() as is. Many core functions have a variant that gets the output and another one that directly outputs it. wp_update_php_annotation() could just call echo wp_get_update_php_annotation().

This way we don't need to modify all the existing function calls, and we keep this a separate change (since it's only required for our 5.2 work, not anything scheduled for 5.1).

I’ll have an updated patch soon. Thanks for the feedback.

[afragen]

updated patch, adding wp_get_update_php_annotation() and echoing it for wp_update_php_annotation()

[afragen]

use <br><span> to avoid extra paragraph tag in plugins list

[afragen]

fixed the missing "

[afragen]

dashboard using span

[afragen]

plugins.php using span

[afragen]

update-core.php using span

[afragen]

So in tracking down this cause of the extra paragraph tag, thanks @aaroncampbell, it seems the wp_plugin_update_row()encloses the row in a paragraph tag. Adding an additional paragraph tag inside of a paragraph tag in not semantic markup and this is what the browser is interpreting as an extra paragraph tag.

Changing to a <br><span> will make this markup work properly in most all areas.

[aaroncampbell]

@flixos90, since you ​mentioned this in Slack, I wanted to ping you here to have you weigh in too. Working with Andy at WordCamp Phoenix contrib day, we tracked down that paragraph tag issue but it seems to me like using <span> and <br> iwould be safe it pretty much any HTML context moving forward, and since the visual aspect is practically unchanged, it feels a lot simpler and saner to keep that markup in the function rather than pass in before and after html.

[flixos90]

@aaroncampbell @afragen
I think <br><span>...</span> is very specific to where the content needs to occur, just as much as <p>...</p> is very specific to where the content needs to occur. One variant is semantically correct in one context, the other one is semantically correct in another. I don't think there is a foundation to argue about one or the other being more applicable generally - forcing any wrapping markup here will get us into trouble with semantics one way or the other, so I think support for $before and $after is necessary.

<p class="description"> and </p> must be the defaults only because they are what is used right now. Not because they fit better than <br><span class="description> and </span> - again, no variant works better than another variant IMO because it all depends on the context.

[afragen]

before and after

[afragen]

@flixos90 that last patch contains the $before and $after. with default to paragraph tags.

I think I will be easier to create another ticket to fix all the wp_update_php_annotation() and wp_get_update_php_annotation() lines as appropriate. #46269

[afragen]

@flixos90 #46275 fixes a few of these issues.

[TimothyBlynJacobs]

The patch looks good to me. Right now its return type is really string|null but is annotated as string. Should the function either return an empty string when unnecessary, or should the doc be updated. I personally prefer returning an empty string, but that's just me.

[afragen]

update return docblock

[afragen]

Return type is now documented as string|null. My preference is to return null. ;-)

[desrosj]

Was there a specific reason for choosing to pass the $before and $after variables to wp_get_update_php_annotation()? I don't hate it, but I don't love it.

Instead, I'd prefer to only pass and handle the markup in wp_update_php_annotation() to keep retrieving the annotation and displaying the annotation separate.

I see the patch on #46269. With my suggested change, the line in wp-admin/update-core.php would go from

$compat .= wp_get_update_php_annotation( '<br><span class="description">', '</span>' );

to

$compat .= '<br><span class="description">' . wp_get_update_php_annotation() . '</span>';

46044.7.diff​ contains that change and a few others:

• Added the missing @since tag detailing the added parameters for wp_update_php_annotation().
• As a tie-breaker vote, I prefer for wp_get_update_php_annotation() to return an empty string. :)

[afragen]

There are several tickets that were based upon this passing a $before and $after parameter. I’m on mobile so I can’t quite see about how this would effect those others.

Your example above for #46269 doesn’t seem to provide and ending span, or does it?

Passing variables to wp_get_update_php_annotation() was to provide for the parameter pass through. As you’ve moved the parameters into the echoing function this may not be necessary. But I’d have to test. Not sure I can get to that until tomorrow. Testing involves looking in all places that plugin updates occur with the annotation present.

Returning an empty string is perfect. 😉

[flixos90]

@desrosj

Was there a specific reason for choosing to pass the $before and $after variables to wp_get_update_php_annotation()? I don't hate it, but I don't love it.

Frankly, we coded ourselves into a corner here a little bit, as wp_update_php_annotation() (already released) outputs markup. Maybe it would have been better if it hadn't, but that is now a point of no return. For that function we need to stay backward-compatible in printing the p tags by default, but now it became obvious that needs to be adjustable, therefore that function needs $before and $after.

For wp_get_update_php_annotation(), I'd argue consistency here is more important than possibly more sanity (especially as the latter is arguable). Therefore I think it should also have $before and $after, and wp_update_php_annotation( $before, $after ) shouldn't do anything more than echo wp_get_update_php_annotation( $before, $after ), passing through the parameters. That is also in line with many other core functions which have such two variants.

@afragen

Return type is now documented as string|null. My preference is to return null.

I strongly recommend returning an empty string as it's a best practice to have as little return types as necessary. Many programming languages even enforce this - I know WordPress doesn't respect such practices often, but I still advocate doing it where we can.

[afragen]

I will update the return type to return and empty string.

@flixos90 I’m not sure why you think we can’t change the functions to that outlined above by @desrosj? Nothing would break and as far as I can tell these functions are only used within core. I’m all for doing it right.

[desrosj]

I agree with @flixos90. If the function shipped accepting parameters already, we should leave the signature the same. Especially where a fatal error will be triggered on newer versions of PHP when too many parameters are passed to a function.

@afragen my plan for today is to land this first, then #46269, then #46275.

[desrosj]

I misread @flixos90's last message. I should have double checked before responding on mobile earlier.

Frankly, we coded ourselves into a corner here a little bit, as wp_update_php_annotation() (already released) outputs markup. Maybe it would have been better if it hadn't, but that is now a point of no return. For that function we need to stay backward-compatible in printing the p tags by default, but now it became obvious that needs to be adjustable, therefore that function needs $before and $after.

The markup is currently output by the wp_update_php_annotation(), but not accepted as parameters. What I am suggesting is:

• wp_update_php_annotation() accepts $before and $after parameters and continues to output markup (<p> by default).
• wp_get_update_php_annotation() only returns the text annotation, with a default of an empty string.

46044.7.diff​ has this.

I looked at other functions in core that have this pattern ($before/$after), there was not a lot of consistency.

• the_title() accepts before and after markup and does not pass them to get_the_title().
• get_the_term_list() accepts before and after markup and does not pass them to get_the_terms().
• comment_author_url_link() accepts before and after and passes them to get_comment_author_url_link().
• the_archive_title() accepts before and after, but does not pass them to get_the_archive_title().
• the_archive_description() accepts before and after, but does not pass them to get_the_archive_description().
• the_date() accepts before and after, but does not pass them to get_the_date().
• the_modified_date() accepts before and after, but does not pass them to get_the_modified_date().

I think there is more of a pattern for not passing the markup down to the get_* functions. I don't feel particularly strong either way, but just wanted to detail my thought process of why I proposed that. What I proposed feels more natural to me.

[desrosj]

Spoke on Slack with @flixos90 about this. He is OK with the method I proposed in 46044.7.diff​.

[desrosj]

In 44935:

General: Improve the PHP update notice annotation.

This change introduces the wp_get_update_php_annotation() function, which returns the message displayed when a host filters the direct PHP update or PHP update education URLs to indicate the information is site specific and provided by the host, not WordPress core.

It also updates wp_update_php_annotation() to accept a $before and $after parameter, which makes this notice more flexible for displaying in multiple locations within the admin area. Previously, the markup output in wp_update_php_annotation() was hardcoded, which was making it difficult to display it properly in multiple locations.

Props afragen, aaroncampbell, flixos90, TimothyBlynJacobs, desrosj.
Fixes #46044.