Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#51123 closed defect (bug) (fixed)

commonL10n and other JS globals removed without backwards compatibility

Reported by: kbjohnson90's profile kbjohnson90 Owned by: ocean90's profile ocean90
Milestone: 5.5.1 Priority: normal
Severity: normal Version: 5.5
Component: Script Loader Keywords: needs-dev-note has-patch commit dev-reviewed fixed-major
Focuses: javascript Cc:

Description

In WordPress 5.5, the localized translations under commonL10n were replaced by wp.i18n.__ without deprecation notice or backwards compatibility. Plugins using commonL10n now have JavaScript errors introduced by updating WordPress which can break site functionality.

Attachments (8)

91063188-a5d56880-e5fb-11ea-843a-82390e42b9d9.png (314.7 KB) - added by kbjohnson90 4 years ago.
commonL10n is not defined
91063188-a5d56880-e5fb-11ea-843a-82390e42b9d9.2.png (314.7 KB) - added by kbjohnson90 4 years ago.
commonL10n is not defined
deprecate_commonl10n.patch (1.2 KB) - added by omarreiss 4 years ago.
deprecate_commonl10n.1.patch (11.8 KB) - added by omarreiss 4 years ago.
51123.diff (12.3 KB) - added by SergeyBiryukov 4 years ago.
51123.2.diff (20.7 KB) - added by peterwilsoncc 4 years ago.
51123.3.diff (13.1 KB) - added by SergeyBiryukov 4 years ago.
51123.4.diff (13.1 KB) - added by SergeyBiryukov 4 years ago.

Download all attachments as: .zip

Change History (54)

@kbjohnson90
4 years ago

commonL10n is not defined

@kbjohnson90
4 years ago

commonL10n is not defined

#1 follow-up: @johnbillion
4 years ago

  • Component changed from General to Script Loader

This was documented in https://make.wordpress.org/core/2020/08/05/more-support-for-javascript-i18n-in-wordpress-5-5/ but it's definitely not as explicit as it could be.

I wonder if there's a way we can lazily proxy these variables to their __() equivalent in the future.

#2 in reply to: ↑ 1 @TimothyBlynJacobs
4 years ago

Replying to johnbillion:

I wonder if there's a way we can lazily proxy these variables to their __() equivalent in the future.

I think we might be able to use Object.defineProperty with a get() function that would proxy.

#3 @kbjohnson90
4 years ago

  • Summary changed from commonL10n removed without deprecation notice or backwards compatibility to commonL10n removed without backwards compatibility

#4 @kbjohnson90
4 years ago

A backwards compatibility layer for removed localized variables could be added to map the configuration to the translated values in JavaScript, as opposed to PHP.

In this case, the re-usable configuration was replaced with a single use, inline translation.

Instead, the configuration value could be updated to use __() while maintaining re-usability.

var commonL10n = {
    dismiss: __( 'Dismiss this notice.' ),
    ...
}

#5 follow-up: @peterwilsoncc
4 years ago

Could something along these lines be added to each affected file?

// First line
window.commonL10n = window.commonL10n || {};

// As needed.
commonL10n.someString = __( 'Some string' );
commonL10n.anotherString = __( 'Another string' );

A back-compat break would remain for any plugins that have filtered the strings upon registration in the script loader, but I suspect that is an edge case.

I am also very slightly concerned such an approach makes the changes in the earlier commits a little redundant as the commonL10n object remains in memory, it simply results in requiring the wp-i18n script as a dependency for each of the files.

#6 in reply to: ↑ 5 @TimothyBlynJacobs
4 years ago

Replying to peterwilsoncc:

I am also very slightly concerned such an approach makes the changes in the earlier commits a little redundant as the commonL10n object remains in memory, it simply results in requiring the wp-i18n script as a dependency for each of the files.

Does wp-i18n cache translations? If not, there might be performance benefit to storing the translated version in an object.

There is also the question of strings that were changed to use _n since it requires the number of results to give a result.

Last edited 4 years ago by TimothyBlynJacobs (previous) (diff)

#7 @joostdevalk
4 years ago

At the very least we need to make sure in 5.5.1 that this doesn’t cause JS errors, as that breaks other plugins / themes / core that need JS.

#8 @SergeyBiryukov
4 years ago

  • Milestone changed from Awaiting Review to 5.5.1

#9 @omarreiss
4 years ago

  • Keywords has-patch added

Added a patch which cleanly deprecates the l10n object getters, throws a console error and returns an empty string whenever a deprecated getter is called.

#10 @SergeyBiryukov
4 years ago

  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

Related: [48267] / #50526.

#11 @ocean90
4 years ago

How many plugins does this really affect? Why/how are they using the variable which is intended for core usage only similar to any other translatable string in core?

#12 @joostdevalk
4 years ago

@ocean90 we've removed a JavaScript global without even proper deprecation. This is a backwards incompatible change, that causes javascript errors, which means that plugins that are not maintained don't just break themselves but can break everything.

If this were PHP, everyone would be crying foul.

#13 @TimothyBlynJacobs
4 years ago

Why __defineGetter__ instead of defineProperty? It seems like the former isn't encouraged, at least according to mdn.

#14 @peterwilsoncc
4 years ago

@TimothyBlynJacobs defineProperty is unavailable in IE11, whereas __defineGetter__ is.

Given the latter is deprecated, it might be preferable to use some conditional code in a wrapper to prevent errors if any browsers hard deprecate the function.

Reviewing other deprecated JavaScript code, the functionality is generally labelled as such in the docblocks but there isn't an equivalent of PHP's _doing_it_wrong() for JavaScript.

In my view, adding such a function is best left for another ticket.

#15 @desrosj
4 years ago

Related: #50999.

This ticket was mentioned in Slack in #core by desrosj. View the logs.


4 years ago

#17 @TimothyBlynJacobs
4 years ago

defineProperty is unavailable in IE11, whereas defineGetter is.

@peterwilsoncc is it a particular part of defineProperty that isn't available? According to MDN it's available, https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/defineProperty.

Reviewing other deprecated JavaScript code, the functionality is generally labelled as such in the docblocks but there isn't an equivalent of PHP's _doing_it_wrong() for JavaScript.

It isn't a _doing_it_wrong, but we could potentially use the deprecated package: https://github.com/WordPress/gutenberg/tree/7fef85b97c13ab59b47d6d6299e2ba94e9fb00b5/packages/deprecated

#18 @peterwilsoncc
4 years ago

@TimothyBlynJacobs Must have confused my MDN reading, sorry boss. In that case, I suggest:

  • remove console logs for consideration for deprecated JS in another ticket
  • switch to defineProperty per Timothy's suggestion above.

#19 follow-up: @ocean90
4 years ago

  • Milestone changed from 5.5.1 to 5.5.2
  • Owner changed from SergeyBiryukov to ocean90

Moving milestone so there's time to identify the possible affected plugins and to discuss other options like updating the plugin/theme.
Returning an empty string might not be the best solution depending on the yet unknown usage of the internal commonL10n strings. Worth to mention that this wasn't the only variable removed during the switch to wp.i18n.

#20 @ocean90
4 years ago

#51171 was marked as a duplicate.

#21 in reply to: ↑ 19 @ocean90
4 years ago

Replying to ocean90:

Worth to mention that this wasn't the only variable removed during the switch to wp.i18n.

In #51171 some research was done for this.

#22 @johnbillion
4 years ago

  • Summary changed from commonL10n removed without backwards compatibility to commonL10n and other JS globals removed without backwards compatibility

From #51171:

  • wp.updates.l10n
  • wpPointerL10n
  • commonl10n
  • userProfileL10n
  • privacyToolsL10n
  • authcheckL10n
  • wp.themePluginEditor.l10n
  • tagsl10n
  • adminCommentsL10n
  • tagsSuggestL10n
  • wpColorPickerL10n
  • attachMediaBoxL10n
  • postL10n
  • inlineEditL10n
  • plugininstallL10n
  • navMenuL10n
  • commentL10n
  • setPostThumbnailL10n

#23 follow-up: @johnbillion
4 years ago

How about wholesale reverting the JS i18n changes in 5.5 and revisit in 5.6?

#24 @joostdevalk
4 years ago

I think deprecating these properly is a better idea. A lot of the changes were absolutely for the better, we should have just been a bit more careful with those globals.

#25 in reply to: ↑ 23 @ocean90
4 years ago

  • Keywords needs-patch added; has-patch removed

Replying to johnbillion:

How about wholesale reverting the JS i18n changes in 5.5 and revisit in 5.6?

Reverting is definitely not an option as it will cause more trouble (no clean revert possible, translators have to re-translate/approve due do strings moving around, reintroducing i18n issues) than providing a simple fallback/deprecation for the few plugins using the globals as suggested by Joost.

#26 @SergeyBiryukov
4 years ago

Should we prioritize the deprecation notices for 5.5.1 while we still can?

I agree these i18n changes are great, it just seems like the dev note could have a bit more details, e.g. the list of the removed globals and properties. This also coincided with jQuery Migrate being disabled in the same release, which causes quite a few issues, so adding some fallback seems like a good idea.

#27 @joostdevalk
4 years ago

@omarreiss expects to have a patch within the hour.

#28 @joostdevalk
4 years ago

  • Keywords needs-dev-note has-patch added; needs-patch removed

#29 @omarreiss
4 years ago

Patch deprecates all properties on all removed globals. I use defineProperty now instead of __defineGetter__. If the global has been redefined by a plugin or theme, the properties will still throw a deprecation error, but will still be returned as redefined.

Last edited 4 years ago by omarreiss (previous) (diff)

@SergeyBiryukov
4 years ago

#30 @SergeyBiryukov
4 years ago

  • Keywords commit added

The approach looks good to me and works as expected in my testing.

Made some minor edits to fix JSHint errors, correct some docs, and make the error message translatable.

#31 @SergeyBiryukov
4 years ago

  • Milestone changed from 5.5.2 to 5.5.1

#32 follow-up: @peterwilsoncc
4 years ago

I'm a little concerned about returning empty strings in place of the original text, especially for important notices such as commonL10n.warnDelete and similar that warn an item is about to be permanently deleted. (cc @SergeyBiryukov following our conversation in Slack regarding this)

In 51123.2.diff

  • Deprecated objects include the original text -- where the translation was changed in the original commit, I've used the new string to avoid causing problems for the translation teams
  • Switched console.error to console.warn as these match deprecation warnings used in the version of React shipped with WordPress, refer https://github.com/facebook/react/issues/9395
  • window.tagsl10n appears to have been named window.tagsSuggestL10n in 5.5 so I have renamed that to match in the updated patch
  • wpWidgets.l10n wasn't included in the previous patches, I have added it to src/js/_enqueues/admin/widgets.js

I'm after logic checks on the following:

  • the use of pagenow for network and single site plugins
  • how I have used the translation functions
  • placement of translations will not cause extra work per @ocean90's concerns above

#33 in reply to: ↑ 32 @ocean90
4 years ago

  • Keywords commit removed

Replying to peterwilsoncc:

Thanks for your work on the patch but I think it's not responsible to ship such a performance penalty in core for everyone.
We're still looking at plugins which are using "private" globals which unfortunately were required to pass core translations to scripts. Just because they are there it doesn't mean that they are supported in any way. Otherwise there would have been some proper documentation. #50976 is a similar recent example for breaking something that wasn't intended to work.
I also took a look at some of the plugins and some don't even define the necessary handle as a script dependencies neither do they the check for their existence before accessing the globals. They just expect them to be there…
With regards to your example commonL10n.warnDelete, that's definitely something we shouldn't have to care about because the proper API would be using window.showNotice.warn().

Though, I do understand the issue that due to a few plugins other plugins or even core may not working properly anymore. And that's something we should fix by providing a graceful fallback as suggested in 51123.diff.
I'm only wondering why the globals have all been defined in common.js? This will make the globals suddenly available on all screens. Wouldn't it be better to add them to each file where they were used previously?

Unlike in PHP, the now deprecated and possible unused code would get shipped to the client on every (uncached) page load. So, we should include this with a clear deprecation policy. For example the current fallback will only be included in the next two major versions but you can install plugin X (similar to Enable jQuery Migrate Helper) to keep the fallback for a bit longer. We could use @wordpress/deprecated for the extended message.

#34 @peterwilsoncc
4 years ago

Yeah, the performance consideration is fair enough and something I started to wonder as I moved toward finishing the patch but I figured I'd share it.

51123.2.diff can be discarded as an interesting experiment.

As 51123.diff is built upon the following will need to be done:

  • window.tagsl10n renamed window.tagsSuggestL10n
  • wpWidgets.l10n will need to be added
  • further work as mentioned above.

Thanks for reviewing @ocean90

#35 @joostdevalk
4 years ago

Unlike in PHP, the now deprecated and possible unused code would get shipped to the client on every (uncached) page load. So, we should include this with a clear deprecation policy.

I fully support this. Two versions seems good to me too, should we do a make post to discuss that? Or can that just be decided in this thread?

#36 @omarreiss
4 years ago

Reasons why I think 51123.diff should be committed for 5.5.1 (with the missed vars @peterwilsoncc noted).

  • It's a no risk change and solves the issues. Even takes into account potential plugins who redefine the removed globals.
  • Returning empty strings is fine. The worst thing that can happen is that these strings (which shouldn't have been used by integrators in the first place) render empty. Much better than breaking execution and actually nudges integrators to update.
  • A deprecation policy for JavaScript is definitely something that should be worked on. Gutenberg already has one that seems to work fine: https://developer.wordpress.org/block-editor/developers/backward-compatibility/deprecations/. There's no reason this should block commit though.
  • The reason I defined most globals in common.js is to keep the fix as simple as possible. common.js gets loaded mostly anywhere in the admin and since these globals are deprecated, we don't have to worry about them being overly present. The impact of that is negligible and it makes cleanup later a more simple task. In places where only an l10n property was removed instead of the entire global, I had to do it in the file where that actually happens, since the global is still localized onto that script.

#37 @SergeyBiryukov
4 years ago

  • Keywords commit added

51123.3.diff addresses a few points from comment:32 and comment:34:

  • Switch console.error() to console.warn().
  • Rename duplicate window.tagsl10n object to window.tagsSuggestL10n.
  • Add missing wpWidgets.l10n object.

I think this is ready for commit now.

#38 follow-up: @desrosj
4 years ago

A few of the variables are missing @since tags.

  • setPostThumbnailL10n
  • navMenuL10n
  • plugininstallL10n
  • postL10n
  • userProfileL10n

I don't think this is a blocker for commit though. RC2 should be going out later today, so having this merged for testing is preferable. The @since values for each of those can be researched and the docs can be updated later.

#39 in reply to: ↑ 38 @SergeyBiryukov
4 years ago

Replying to desrosj:

I don't think this is a blocker for commit though. RC2 should be going out later today, so having this merged for testing is preferable. The @since values for each of those can be researched and the docs can be updated later.

Thanks for the review! 51123.4.diff adds the missing @since tags.

#40 @SergeyBiryukov
4 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 48923:

Script Loader: Add backward compatibility for JavaScript i18n globals and properties deprecated in WordPress 5.5.

The recommended approach for any plugins using these globals or properties is to switch to the newer wp.i18n functions.

In the meantime, this ensures that accessing any of these globals does not break the rest of the code on the page, and an appropriate warning message is logged to the JavaScript console.

Follow-up to: https://core.trac.wordpress.org/query?summary=~wp.i18n&milestone=5.5

Props omarreiss, peterwilsoncc, kbjohnson90, johnbillion, TimothyBlynJacobs, joostdevalk, ocean90, desrosj, SergeyBiryukov.
Fixes #51123.

#41 @SergeyBiryukov
4 years ago

  • Keywords dev-feedback fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for backporting to the 5.5 branch after a second committer's review.

#43 @desrosj
4 years ago

  • Keywords dev-reviewed added; dev-feedback removed

Looks good to backport.

#44 @desrosj
4 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 48925:

Script Loader: Add backward compatibility for JavaScript i18n globals and properties deprecated in WordPress 5.5.

The recommended approach for any plugins using these globals or properties is to switch to the newer wp.i18n functions.

In the meantime, this ensures that accessing any of these globals does not break the rest of the code on the page, and an appropriate warning message is logged to the JavaScript console.

Follow-up to: https://core.trac.wordpress.org/query?summary=~wp.i18n&milestone=5.5

Props omarreiss, peterwilsoncc, kbjohnson90, johnbillion, TimothyBlynJacobs, joostdevalk, ocean90, desrosj, SergeyBiryukov.
Merges [48923] to the 5.5 branch.
Fixes #51123.

This ticket was mentioned in Slack in #core by planningwrite. View the logs.


4 years ago

Note: See TracTickets for help on using tickets.