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

(2P) Revert "Revert "FSE: Add API endpoint to side-load images"" #35344

Merged
merged 2 commits into from
Aug 16, 2019

Conversation

obenland
Copy link
Member

@obenland obenland commented Aug 13, 2019

Reverts #35328
Adds proper namespacing.

@obenland obenland added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Type] Task [Goal] Full Site Editing labels Aug 13, 2019
@obenland obenland added this to the Ajax: Iteration 7 milestone Aug 13, 2019
@obenland obenland requested review from a team as code owners August 13, 2019 17:24
@matticbot
Copy link
Contributor

@obenland obenland added [Status] In Progress and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Aug 13, 2019
@obenland obenland changed the title Revert "Revert "FSE: Add API endpoint to side-load images"" (2P) Revert "Revert "FSE: Add API endpoint to side-load images"" Aug 13, 2019
@obenland obenland added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Aug 13, 2019
@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

Copy link
Contributor

@getdave getdave left a comment

Choose a reason for hiding this comment

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

I've compared against the diff that reverted these changes and it looks to be a correct revert of that revert (help me!).

Just to confirm you've moved everything into the A8C\FSE namespace right?

* @package A8C\FSE
*/

namespace A8C\FSE;
Copy link
Contributor

@getdave getdave Aug 13, 2019

Choose a reason for hiding this comment

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

So here is where you've moved the class into the namespace?

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct

public function register_rest_api() {
require_once __DIR__ . '/class-wp-rest-sideload-image-controller.php';

( new WP_REST_Sideload_Image_Controller() )->register_routes();
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to confirm there is no global namespace qualifier here because the WP_REST_Sideload_Image_Controller class is now in the A8C\FSE namespace?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, no qualifier needed because the current namespace also applies to WP_REST_Sideload_Image_Controller.

@obenland obenland merged commit b7e8ff7 into master Aug 16, 2019
obenland added a commit that referenced this pull request Aug 16, 2019
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Aug 16, 2019
@marekhrabe marekhrabe deleted the revert-35328-revert-34823-add/fse-image-endpoint branch August 19, 2019 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants