Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Plugin: Bump minimum WordPress version to 5.3 #20628

Merged
merged 6 commits into from
Mar 6, 2020

Conversation

mcsf
Copy link
Contributor

@mcsf mcsf commented Mar 4, 2020

Description

Fixes #20484

With the release of RC1 for WordPress 5.4, and given the constraints raised in #20484, this PR bumps the required WordPress version for running the Gutenberg plugin from 5.2 to 5.3.

In the process, this PR eliminates the interception of core's block_editor_preload_paths filter, which was in place to provide compatibility for the following features:

@talldan, @aduth: As authors of the aforementioned PRs, could you confirm these removals for me?

How has this been tested?

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@mcsf mcsf added the Gutenberg Plugin Issues or PRs related to Gutenberg Plugin management related efforts label Mar 4, 2020
@mcsf mcsf requested review from talldan and aduth March 4, 2020 17:17
@github-actions
Copy link

github-actions bot commented Mar 4, 2020

Size Change: -2.26 kB (0%)

Total Size: 863 kB

Filename Size Change
build/annotations/index.js 3.43 kB -1 B
build/api-fetch/index.js 3.39 kB -1 B
build/block-editor/index.js 104 kB -411 B (0%)
build/block-editor/style-rtl.css 10.5 kB +24 B (0%)
build/block-editor/style.css 10.5 kB +21 B (0%)
build/block-library/editor-rtl.css 7.26 kB -100 B (1%)
build/block-library/editor.css 7.26 kB -100 B (1%)
build/block-library/index.js 115 kB -776 B (0%)
build/blocks/index.js 57.7 kB +50 B (0%)
build/components/index.js 191 kB -126 B (0%)
build/components/style-rtl.css 15.5 kB -18 B (0%)
build/components/style.css 15.5 kB -12 B (0%)
build/compose/index.js 5.75 kB -2 B (0%)
build/date/index.js 5.36 kB -4 B (0%)
build/dom-ready/index.js 569 B +1 B
build/edit-post/index.js 91.3 kB -213 B (0%)
build/edit-post/style-rtl.css 8.71 kB +41 B (0%)
build/edit-post/style.css 8.71 kB +44 B (0%)
build/edit-site/style-rtl.css 2.48 kB -28 B (1%)
build/edit-site/style.css 2.48 kB -27 B (1%)
build/edit-widgets/style-rtl.css 2.59 kB -2 B (0%)
build/edit-widgets/style.css 2.58 kB -2 B (0%)
build/editor/index.js 43.8 kB -117 B (0%)
build/escape-html/index.js 734 B +1 B
build/format-library/index.js 7.11 kB -492 B (6%)
build/hooks/index.js 1.92 kB +1 B
build/keyboard-shortcuts/index.js 2.3 kB +1 B
build/list-reusable-blocks/index.js 2.99 kB +1 B
build/nux/index.js 3.01 kB -16 B (0%)
build/plugins/index.js 2.54 kB -1 B
build/redux-routine/index.js 2.84 kB +1 B
build/rich-text/index.js 14.3 kB +1 B
build/server-side-render/index.js 2.55 kB +6 B (0%)
build/url/index.js 4 kB -1 B
build/warning/index.js 1.14 kB -1 B
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.01 kB 0 B
build/autop/index.js 2.58 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.02 kB 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 760 B 0 B
build/block-library/style-rtl.css 7.5 kB 0 B
build/block-library/style.css 7.51 kB 0 B
build/block-library/theme-rtl.css 669 B 0 B
build/block-library/theme.css 671 B 0 B
build/block-serialization-default-parser/index.js 1.65 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/core-data/index.js 10.6 kB 0 B
build/data-controls/index.js 1.03 kB 0 B
build/data/index.js 8.22 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom/index.js 3.06 kB 0 B
build/edit-site/index.js 4.64 kB 0 B
build/edit-widgets/index.js 4.42 kB 0 B
build/editor/editor-styles-rtl.css 325 B 0 B
build/editor/editor-styles.css 327 B 0 B
build/editor/style-rtl.css 3.98 kB 0 B
build/editor/style.css 3.98 kB 0 B
build/element/index.js 4.45 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.48 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keycodes/index.js 1.68 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 4.85 kB 0 B
build/notices/index.js 1.57 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/primitives/index.js 1.49 kB 0 B
build/priority-queue/index.js 780 B 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/viewport/index.js 1.61 kB 0 B
build/wordcount/index.js 1.18 kB 0 B

compressed-size-action

@Soean
Copy link
Member

Soean commented Mar 4, 2020

We also can remove gutenberg_safe_style_css_column_flex_basis and maybe gutenberg_register_vendor_scripts? @gziolo

@talldan
Copy link
Contributor

talldan commented Mar 5, 2020

@mcsf That looks good to me, thanks for handling that!

@gziolo
Copy link
Member

gziolo commented Mar 5, 2020

We also can remove gutenberg_safe_style_css_column_flex_basis and maybe gutenberg_register_vendor_scripts? @gziolo

I would keep gutenberg_register_vendor_scripts because we are going to bump React and lodash in the future and we need to test that in the plugin first. Those versions align with WP core but at least React had 4 minor releases in the meantime. We can remove lodash from the function because it's still the latest version.

@Soean
Copy link
Member

Soean commented Mar 5, 2020

@gziolo Ok, then we should adjust the the TODO inside the function:

// TODO: Overrides for react, react-dom and lodash are necessary
// until WordPress 5.3 is released.

@mcsf mcsf force-pushed the update/minimum-wp-version-53 branch from 8bdd5ee to 9ac1e2a Compare March 5, 2020 15:03
@mcsf
Copy link
Contributor Author

mcsf commented Mar 5, 2020

@Soean, @gziolo: I've pushed 1018968 and 9ac1e2a, please have a look. 🙇 For the TODO note, I'll let you work it out.

@aduth
Copy link
Member

aduth commented Mar 5, 2020

@talldan, @aduth: As authors of the aforementioned PRs, could you confirm these removals for me?

Confirmed via: https://github.com/WordPress/WordPress/blob/5.3/wp-admin/edit-form-blocks.php#L51

@aduth
Copy link
Member

aduth commented Mar 5, 2020

I don't think we need the React or Lodash registrations? As of WordPress 5.3, the versions are the same as what we're overriding:

https://github.com/WordPress/WordPress/blob/5.3/wp-includes/script-loader.php#L94-L97

To @gziolo's point in #20628 (comment), it might be something where we want to keep the function for the sake of anticipating some future use. I don't feel strongly one way or the other, but it would be easy enough to refer back to the history of the file to restore the function if and when it's needed once more.

@mcsf
Copy link
Contributor Author

mcsf commented Mar 5, 2020

I really don't feel strongly about this; I can either remove gutenberg_register_vendor_scripts entirely, or hollow it out, or something else. State your preferences. :)

@aduth
Copy link
Member

aduth commented Mar 5, 2020

I'd suggest to just remove it altogether.

@mcsf
Copy link
Contributor Author

mcsf commented Mar 5, 2020

I'd suggest to just remove it altogether.

I starting doing this, then realised that gutenberg_register_vendor_scripts is used by the script bin/get-vendor-scripts.php, itself used by bin/build-plugin-zip.sh. I could remove get-vendor-scripts and remove the logic in build-plugin-zip to skip that step, but at this point I'm thinking that it's a lot of useful machinery that we might bring back soon. Despite YAGNI, now I'm leaning more towards emptying the function and leaving it in place, so that every other piece can still rely on it.

I've tentatively pushed 1be218b. Thoughts?

@aduth
Copy link
Member

aduth commented Mar 5, 2020

Fair enough. I don't feel too strongly about it, and agree that it would be a pain to bring everything back in place at a later point. One suggestion I might make is to include an inline code comment explaining why we have an empty function there.

@gziolo
Copy link
Member

gziolo commented Mar 5, 2020

Agreed, a code comment with a link to this PR should make it easier to restore code when we need another override in the future.

@mcsf
Copy link
Contributor Author

mcsf commented Mar 6, 2020

Done.

@aduth
Copy link
Member

aduth commented Mar 6, 2020

I'm glad you raised the point about the plugin build script and its implementation for downloading vendor dependencies. In revisiting my own #20225, I sense that the use of gutenberg_register_vendor_script there would not be sufficient to account for the polyfill in the plugin build. It's already the case prior to that pull request, too:

https://plugins.trac.wordpress.org/browser/gutenberg/tags/7.6.0/vendor
https://plugins.trac.wordpress.org/browser/gutenberg/tags/7.6.0/lib/compat.php#L58
https://plugins.trac.wordpress.org/browser/gutenberg/tags/7.6.0/lib/compat.php#L101

Trying to think how best to address this (without also needlessly blocking this pull request or #20225). I imagine the solution would be one of either (a) move these gutenberg_register_vendor_script into gutenberg_register_vendor_scripts or (b) update the implementation of get-vendor-scripts.php to account for more than just gutenberg_register_vendor_scripts.

Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per my previous comment, I think it will be easiest to merge this, then I can rebase my other pull request with the necessary changes to account for the polyfill vendor scripts.

The changes here look sensible to me 👍

@mcsf
Copy link
Contributor Author

mcsf commented Mar 6, 2020

Thanks, I appreciate the deep dive and unblocking.

@mcsf mcsf merged commit 3ab3727 into master Mar 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Gutenberg Plugin Issues or PRs related to Gutenberg Plugin management related efforts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gutenberg is unintentionally incompatible with WordPress 5.2.x.
5 participants