Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#46044 closed defect (bug) (fixed)

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

Reported by: afragen's profile afragen Owned by: desrosj's profile desrosj
Milestone: 5.2 Priority: normal
Severity: normal Version: 5.1
Component: Site Health Keywords: has-patch servehappy dev-feedback commit
Focuses: Cc:

Description

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

Attachments (10)

46044.diff (3.3 KB) - added by afragen 5 years ago.
46044.2.diff (1.4 KB) - added by afragen 5 years ago.
updated patch, adding wp_get_update_php_annotation() and echoing it for wp_update_php_annotation()
46044.3.diff (1.3 KB) - added by afragen 5 years ago.
use <br><span> to avoid extra paragraph tag in plugins list
46044.4.diff (1.3 KB) - added by afragen 5 years ago.
fixed the missing "
screenshot_01.png (60.6 KB) - added by afragen 5 years ago.
dashboard using span
screenshot_02.png (50.4 KB) - added by afragen 5 years ago.
plugins.php using span
screenshot_03.png (51.6 KB) - added by afragen 5 years ago.
update-core.php using span
46044.5.diff (1.7 KB) - added by afragen 5 years ago.
before and after
46044.6.diff (1.7 KB) - added by afragen 5 years ago.
update return docblock
46044.7.diff (1.6 KB) - added by desrosj 5 years ago.

Download all attachments as: .zip

Change History (31)

@afragen
5 years ago

#1 @afragen
5 years ago

  • Version set to trunk

#2 follow-up: @flixos90
5 years ago

  • Milestone changed from Awaiting Review to 5.2

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).

#3 in reply to: ↑ 2 @afragen
5 years ago

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
5 years ago

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

#4 @afragen
5 years ago

  • Summary changed from Rename `wp_update_php_annotation()` to `wp_get_update_php_annotation()` to Add `wp_get_update_php_annotation()` to return the Update PHP annotation

@afragen
5 years ago

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

@afragen
5 years ago

fixed the missing "

@afragen
5 years ago

dashboard using span

@afragen
5 years ago

plugins.php using span

@afragen
5 years ago

update-core.php using span

This ticket was mentioned in Slack in #core-php by afragen. View the logs.


5 years ago

#6 @afragen
5 years ago

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.

#7 @aaroncampbell
5 years ago

@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.

#8 @flixos90
5 years ago

@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
5 years ago

before and after

#9 @afragen
5 years ago

@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

#10 @afragen
5 years ago

@flixos90 #46275 fixes a few of these issues.

Last edited 5 years ago by afragen (previous) (diff)

#11 @TimothyBlynJacobs
5 years ago

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
5 years ago

update return docblock

#12 @afragen
5 years ago

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

@desrosj
5 years ago

#13 @desrosj
5 years ago

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. :)

#14 @afragen
5 years ago

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. 😉

Last edited 5 years ago by afragen (previous) (diff)

#15 @flixos90
5 years ago

@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.

#16 @afragen
5 years ago

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.

#17 @desrosj
5 years ago

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 was to land this first, then #46269, then #46275.

Version 0, edited 5 years ago by desrosj (next)

#18 @desrosj
5 years ago

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.

#19 @desrosj
5 years ago

  • Keywords commit added

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

#20 @desrosj
5 years ago

  • Owner set to desrosj
  • Resolution set to fixed
  • Status changed from new to closed

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.

#21 @spacedmonkey
5 years ago

  • Component changed from Plugins to Site Health
Note: See TracTickets for help on using tickets.