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

Add experiment for client-side media processing #64650

Merged
merged 33 commits into from
Aug 27, 2024

Conversation

swissspidy
Copy link
Member

@swissspidy swissspidy commented Aug 20, 2024

What?

See #61447 for full context.

Split out from #64278, this adds:

  • A new checkbox on the experiments page for client-side media processing
  • A first set of REST API changes required for the feature
  • Foundation for configuring cross-origin isolation on the editor an adding crossorigin attributes where required

Submitting this PHP-heavy change to make subsequent reviews easier.

Again, this is all behind an experiment. Iterations can be made in subsequent PRs :-)

These changes are all very well tested as part of https://github.com/swissspidy/media-experiments

All these changes are only taking in effect when the experiment is enabled on the settings page. Nothing happens otherwise.

Why?

This is just a small part towards client-side media processing in Gutenberg. See #61447 for full context.

How?

Testing Instructions

There should be a new checkbox on the settings page. No visual changes otherwise.

Testing Instructions for Keyboard

N/A

Screenshots or screencast

N/A

@swissspidy swissspidy added [Type] Enhancement A suggestion for improvement. [Feature] Media Anything that impacts the experience of managing media labels Aug 20, 2024
Copy link

github-actions bot commented Aug 20, 2024

Flaky tests detected in 7278d79.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10548153719
📝 Reported issues:

@swissspidy swissspidy force-pushed the fix/61447-add-experiment branch 2 times, most recently from eec3eef to 9d9848e Compare August 21, 2024 14:51
@swissspidy swissspidy added the No Core Sync Required Indicates that any changes do not need to be synced to WordPress Core label Aug 23, 2024
@swissspidy swissspidy force-pushed the fix/61447-add-experiment branch 2 times, most recently from 8fe08b7 to fd0303e Compare August 23, 2024 12:54
@swissspidy
Copy link
Member Author

swissspidy commented Aug 23, 2024

Note: the only PHP unit test that's failing is because of #64743. Once that PR is merged, it should be fine.

@swissspidy swissspidy marked this pull request as ready for review August 23, 2024 14:10
Copy link

github-actions bot commented Aug 23, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: swissspidy <swissspidy@git.wordpress.org>
Co-authored-by: felixarntz <flixos90@git.wordpress.org>
Co-authored-by: spacedmonkey <spacedmonkey@git.wordpress.org>
Co-authored-by: TimothyBJacobs <timothyblynjacobs@git.wordpress.org>
Co-authored-by: adamsilverstein <adamsilverstein@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

@swissspidy This looks great so far. Left a number of comments with little things to improve, but nothing major.

lib/load.php Outdated Show resolved Hide resolved
lib/experimental/media/load.php Show resolved Hide resolved
lib/experimental/media/load.php Show resolved Hide resolved
lib/experiments-page.php Outdated Show resolved Hide resolved
Copy link
Member

@TimothyBJacobs TimothyBJacobs left a comment

Choose a reason for hiding this comment

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

This looks good to me from a REST API perspective.

@swissspidy
Copy link
Member Author

@spacedmonkey Could you please re-review after the latest changes? Your stale review is blocking the merge.

Copy link
Member

@adamsilverstein adamsilverstein left a comment

Choose a reason for hiding this comment

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

Fantastic! Left a few questions.

Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

Looks great to me!

@spacedmonkey
Copy link
Member

@swissspidy Provided a review already, but I comments seem to losted in the shuffle. Can you take a look at my last comments.

@swissspidy
Copy link
Member Author

@spacedmonkey All comments were addressed and/or resolved, that's why I am asking for your re-review to unblock this merge. Further changes can be easily done in follow-up PRs, as everything here is behind an experimental flag anyway.

Copy link
Member

@spacedmonkey spacedmonkey left a comment

Choose a reason for hiding this comment

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

LGTM. Good work @swissspidy

@swissspidy swissspidy changed the title Add experiment and REST API changes for client-side media processing Add experiment for client-side media processing Aug 27, 2024
@swissspidy swissspidy merged commit 23db6dc into trunk Aug 27, 2024
62 checks passed
@swissspidy swissspidy deleted the fix/61447-add-experiment branch August 27, 2024 16:12
@github-actions github-actions bot added this to the Gutenberg 19.2 milestone Aug 27, 2024
bph pushed a commit to bph/gutenberg that referenced this pull request Aug 31, 2024
Co-authored-by: swissspidy <swissspidy@git.wordpress.org>
Co-authored-by: felixarntz <flixos90@git.wordpress.org>
Co-authored-by: spacedmonkey <spacedmonkey@git.wordpress.org>
Co-authored-by: TimothyBJacobs <timothyblynjacobs@git.wordpress.org>
Co-authored-by: adamsilverstein <adamsilverstein@git.wordpress.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Media Anything that impacts the experience of managing media No Core Sync Required Indicates that any changes do not need to be synced to WordPress Core [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants