Change History (33)

In the admin bar, title attribute of the ab-item link contains the same text as the screen-reader-text ("1 Plugin Update"). When the update count changes, the title attribute gets the new count (n) but loses the localized text. The same is true of the update count title attribute in the admin menu.

Updating the screen reader text with the new count only is easy (see attached patch).

Updating all three locations with the correct string would require significantly more work (an AJAX call to get the correct localized string?).

The strings generated by wp_get_update_data() will be incorrect until all updates have finished - the data is cached, and isn't regenerated until the end of the update process.

The iframe method is used, instead of an XHR-based system, because we're able to control the upgrade environment much more easily that way.

You can see an example of sending translated strings to JS in wp-admin/nav-menu.php, search for wp_localize_script. The menus data object created by this call can be seen used in wp-admin/js/nav-menu.js, search for menus. to see instances of it. In this case, you'd create an array of the strings in PHP, and call it like so:

wp_localize_script( 'updates', 'updates', $update_strings );

You could then access the updates data object in the JS, to get the strings. These strings will only be loaded when updates.js is loaded.

@paulschreiber thanks for working on this. your patch seemed so very close to working!

its amazing how complicated it is when seeming so simple at first glance. I tried working with your patch and discovered, as @pento pointed out, that the count is cached and not updated at the point you were regenerating the title string.

29022.diff​ takes a different approach, I think this is what pento was talking about?

localize the singular and plural forms of 'plugin', 'theme', and 'translation'

pass the individual counts to the JavaScript as data attributes of a hidden span

decrement the appropriate count based on upgradeType

reconstruct the title/screen reader screen string based on these counts and translated words

I think the patch works correctly, though it needs more testing. Its also a bit verbose with a bunch of repeated code that should be refactorable into something a bit more succinct. @paulschreiber since you are already familiar with the code here, can you take a look at the patch to improve & test?

The strings need to keep the %d (ie, "%d Plugin Updates"). Some languages will move the number to a different location in the string, so we can't just do a string concat in the JS. Instead, we can search and replace the %d, like what already happens in nav-menu.js.

Because the strings are used in update.js, not plugin-install.js, the $scripts->localize() should be for install.

Instead of including the numbers in a hidden span, they can also be included as part of the $scripts->localize() call. This is the standard method for passing this kind of data.

The strings need to keep the %d (ie, "%d Plugin Updates"). Some languages will move the number to a different location in the string, so we can't just do a string concat in the JS. Instead, we can search and replace the %d, like what already happens in nav-menu.js.

Because the strings are used in update.js, not plugin-install.js, the $scripts->localize() should be for install.

Instead of including the numbers in a hidden span, they can also be included as part of the $scripts->localize() call. This is the standard method for passing this kind of data.

This is starting to shape up nicely. Good work on finding each other at WCNYC! :-)

I think each of the count decrement / string generation chunks could be merged into a single loop - the code is all the same, just with different variable names, and a special case for wordpress updates only having the single string. We could also remove reduceCount() if its functionality is reduced to a single use.

Now that we have the count data stored on updatesL10n, I think we could also use that for the bits at the start of decrementCount() - it's better to use a canonical data source than to parse a number from the HTML.

decrementCount funciton got removed as well, since its one line in the loop; WordPress updates treated the same as the rest, except singular and plural are the same, since there can't be a plural WordPress update count.

This is getting better, I'd love to remove all those parseInt's necessitated by wp_localize_script's annoying conversion of ints to strings. Hopefully, we can get #25280 in soon!

That's a good point, I think we'll need a different approach to handle plurals properly.

We have a few options, but I'm not sure which is the least insane:

Add ​Jed to core. We can use this to properly handle the multiple plural forms, with minimum modification to existing code.

After an update has happened, manipulate the appropriate transient that wp_get_update_data() uses to generate the string, then WP_Upgrader_Skin::decrement_update_count can call wp_get_update_data(), send the string to the browser, then the string can be updated easily.

Same as above, but call wp_version_check()/wp_update_plugins()/wp_update_themes() to update the transients.

Alternatively, we can go for the easier option of just setting the string to "Updates available", and not bothering trying to generate an accurate string.

Jed looks cool and worth adding - we are going to face this issue with increasing frequency as we add more JavaScript/Backbone to core.

The problem with relying on the php translation function is that the updates occur in an iframe and there can be multiple updates, the JavaScript runs on the page without a refresh. Hitting the php would likdely require an ajax call.

I like the last, simplest option: reconstruct the string so we don't have to worry about plurals. Not sure this works correctly in all languages, I'm ok with that even if the resulting string isn't perfectly grammatically correct. The code would be greatly simplified and the resulting string would still convey the important information.

Something like:

Updates available - Themes: 3, Plugins: 1, Wordpress: 1

Even when only one update remains, the string still makes perfect sense:

Updates available - Themes: 1

Doing it this way eliminates the translations entirely, what do you think?

That's a good point, I think we'll need a different approach to handle plurals properly.

We have a few options, but I'm not sure which is the least insane:

Add ​Jed to core. We can use this to properly handle the multiple plural forms, with minimum modification to existing code.

After an update has happened, manipulate the appropriate transient that wp_get_update_data() uses to generate the string, then WP_Upgrader_Skin::decrement_update_count can call wp_get_update_data(), send the string to the browser, then the string can be updated easily.

Same as above, but call wp_version_check()/wp_update_plugins()/wp_update_themes() to update the transients.

Alternatively, we can go for the easier option of just setting the string to "Updates available", and not bothering trying to generate an accurate string.

Instead of adding a new string, or a new way of generating the string, attachment:29022.7.diff​ reuses the PHP string generator after each item has been updated, by subtracting a non-permanent cache from the number of items to updated.

This feels pretty close to the "lightest touch" change that we could make to fix this bug.