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

Move REST API endpoints to controllers #3648

Closed
1 task done
JakePT opened this issue Sep 22, 2023 · 5 comments · Fixed by #3650
Closed
1 task done

Move REST API endpoints to controllers #3648

JakePT opened this issue Sep 22, 2023 · 5 comments · Fixed by #3650
Assignees
Milestone

Comments

@JakePT
Copy link
Contributor

JakePT commented Sep 22, 2023

Is your enhancement related to a problem? Please describe.

Following #3643 we should move our other REST API endpoints into their own controllers, for better separation of concerns.

Designs

No response

Describe alternatives you've considered

No response

Code of Conduct

  • I agree to follow this project's Code of Conduct
@JakePT
Copy link
Contributor Author

JakePT commented Sep 22, 2023

To discuss:

  • Whether we should instantiate the API endpoints in their Features, as they are currently, or whether they should be instantiated from the main plugin file. If the latter, what's the best method to do that?
  • Whether we can rename or split some of the endpoints, eg. splitting the Facet endpoints into separate taxonomy/meta endpoints/controllers, and whether the related posts endpoint should be moved to the ElasticPress namespace.

@JakePT JakePT linked a pull request Sep 22, 2023 that will close this issue
4 tasks
@JakePT
Copy link
Contributor Author

JakePT commented Sep 26, 2023

Proceed with putting controllers in REST directory. Rename some endpoints to be feature-agnostic, keep old routes for backwards compatibility.

@JakePT
Copy link
Contributor Author

JakePT commented Sep 28, 2023

@felipeelia I’m trying to resolve failing tests with the PR #3650, but I’m getting an unexpected error when syncing. Syncing is failing because there’s an error output in the response:

Warning: Undefined array key "properties" in /var/www/html/wp-content/plugins/elasticpress/includes/classes/Indexable.php on line 1208

The relevant line is here:

$meta_fields = $mapping[ $this->get_index_name( $blog_id ) ]['mappings']['properties']['meta']['properties'];

But I don’t see anything in the PR that would mess with that: https://github.com/10up/ElasticPress/pull/3650/files
Do you have any idea what the problem might be? I can’t replicate it on the 5.0.0 branch.

@felipeelia
Copy link
Member

felipeelia commented Sep 28, 2023

@JakePT That happens because ElasticPress\REST\MetaRange::get_args() is being called on every load, because WP needs the args parameter to register the REST route. That will need to change anyway, as we really don't want to call Elasticsearch on every page load.

ps.: Adding error_log( wp_debug_backtrace_summary() ); to that method helped me understand why that was being called in the first place.

@felipeelia
Copy link
Member

Closed by #3650

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants