#42238 closed defect (bug) (fixed)
Notice when Rest Comments controler is checking permision for a non existent post type
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 5.4 | Priority: | normal |
Severity: | normal | Version: | 4.7 |
Component: | REST API | Keywords: | good-first-bug has-patch has-unit-tests commit |
Focuses: | Cc: |
Description
The problem comes when I try to make a rest get request for the comments. The request is something like that http://demo.wp-api.org/wp-json/wp/v2/comments ( taken from the docs, in my local environment ), made in postman.
The function check_read_post_permission is usign WP_REST_Posts_Controller to get a post controller based on the post type.
<?php protected function check_read_post_permission( $post, $request ) { $posts_controller = new WP_REST_Posts_Controller( $post->post_type ); $post_type = get_post_type_object( $post->post_type );
Next, the WP_REST_Posts_Controller constructor tries to find the post type object in order to establish the rest_base variable. The problem is that the post type doesn't exists anymore ( for me this happened because I deactivate a plugin, woocommerce in my case, but I think this could happen in other cases also ). So, the get_post_type_object function returns null and than it tries to get a property from a null object -> Notice: Trying to get property of non-object in
...
<?php public function __construct( $post_type ) { $this->post_type = $post_type; $this->namespace = 'wp/v2'; $obj = get_post_type_object( $post_type ); //$obj is null $this->rest_base = ! empty( $obj->rest_base ) ? $obj->rest_base : $obj->rest_base; //notice is thrown
Obviously since I am on my local machine, the WP_DEBUG was set on true and if I set it on false, the notice doesn't appear anyomore. But still, I think it's a small issue, that could be solved in a more elegant way.
To solve this I would probably suggest a check for null on the $obj and maybe in that case rest_base could become a default value or an error should be thrown ( I am not sure what would be the best approach )
If there is someone that needs more information on this, let me know and I will do my best to help.
Attachments (5)
Change History (31)
#2
@
8 years ago
Hello, @subrataemfluence
Thank you for looking into the matter and sorry for not beeing able to respond earilier.
I remade the test with a custom plugin in the same way you did ( without using woocommerce ) and the notice was still there.
Your first few steps were ok ( 1 - 3 ), from than I think it was a misunderstanding. Basically I was speaking of getting the comments from the post using the rest api. I think your example refers to get the posts created with that custom post type.
So, my error is when getting the comments using the rest api and in the comments list there is a comment on a post of a custom post type that not longer exists.
Your next steps in debugging would be: ( the post type needs to be set to accept comments and enable rest api). I used this function to create my custom post type
<?php register_post_type( 'bug_post', array( 'labels' => array( 'name' => __( 'Bug Post' ), 'singular_name' => __( 'Bugs' ) ), 'public' => true, 'has_archive' => true, 'supports' => array( 'title', 'editor', 'author', 'excerpt', 'comments', 'revisions' ), 'show_in_rest' => true, ) );
4.After publishing the post, go on it and add a few comments.
5.Run the next rest route http://{insert_your_local_url}/wp-json/wp/v2/comments -> you should get the list of comments
6.Disable the plugin
7.Run the rest route again -> a PHP notice will appear ( only when WP_DEBUG is set to true )
I hope that this clarifies the issue.
Thank you for the heads up to not use a third party plugin in my testing.
If there is anything else that I can clear please feel free to comment on my ticket.
Thanks,
Dragos
#4
@
8 years ago
Thank you for driving me to the right direction. I now can reproduce the issue. I am trying to dig more into it.
#7
@
8 years ago
Hello @dragosh635, I am submitting a patch which works for me. When you have time can you please verify whether the same works for you as well?
Thank you!
#8
follow-up:
↓ 9
@
8 years ago
Hello, @subrataemfluence
The patch you submitted works for me also and I think it's a good solution.
Please keep me posted if it was accepted.
Dragos
#9
in reply to:
↑ 8
@
8 years ago
Thank you for testing the patch and confirming that it works for you too!
Replying to dragosh635:
Hello, @subrataemfluence
The patch you submitted works for me also and I think it's a good solution.
Please keep me posted if it was accepted.
Dragos
This ticket was mentioned in Slack in #core-restapi by timothybjacobs. View the logs.
6 years ago
#11
@
6 years ago
- Owner set to TimothyBlynJacobs
- Status changed from new to accepted
Going to take a look at this. Is related to #40383.
#12
follow-up:
↓ 13
@
5 years ago
- Keywords needs-patch needs-unit-tests good-first-bug added; has-patch removed
- Milestone changed from Awaiting Review to Future Release
- Version changed from 4.8.2 to 4.7
Thanks for the patch @subrataemfluence! And apologies for the delay in getting to your ticket @dragosh635.
Looking at the code, WP_REST_Posts_Controller::check_read_permission()
will return false if the post type isn't registered. So instead of trying to adjust the WP_REST_Posts_Controller
constructor to allow for invalid post types, I think it'd be best to short circuit WP_REST_Comments_Controller::check_read_post_permission()
and return false if the post type does not exist.
@subrataemfluence are you interested in writing a new patch?
#13
in reply to:
↑ 12
@
5 years ago
Thank you @TimothyBlynJacobs for giving me the opportunity. Unfortunately I am travelling right now and won't be able to work on this for next few days. I am fine and actually would be great if somebody else completes it. :)
Replying to TimothyBlynJacobs:
Thanks for the patch @subrataemfluence! And apologies for the delay in getting to your ticket @dragosh635.
Looking at the code,
WP_REST_Posts_Controller::check_read_permission()
will return false if the post type isn't registered. So instead of trying to adjust theWP_REST_Posts_Controller
constructor to allow for invalid post types, I think it'd be best to short circuitWP_REST_Comments_Controller::check_read_post_permission()
and return false if the post type does not exist.
@subrataemfluence are you interested in writing a new patch?
#14
@
5 years ago
- Keywords has-patch added; needs-patch removed
Hello @TimothyBlynJacobs
I have created a patch the way you suggested. It works at my end. When you have time can you please verify whether the same works for you as well?
CC: @dragosh635, @subrataemfluence
Thanks in advance!
#15
@
5 years ago
Thanks for working on a patch @imani3011!
Some minor corrections:
We can just check if the value is non-falsey instead of explicitly checking that the value is_null
. For example: https://github.com/WordPress/wordpress-develop/blob/master/src/wp-admin/edit.php#L35
WordPress also has slightly different PHP coding standards. So there should be a space around the parentheses.
Are you interested in writing a unit test to test this fix? It could be added to the WP_Test_REST_Comments_Controller
test class.
#16
@
5 years ago
@imani3011, Thank you for submitting the patch. Just a quick note about the naming convention for patch versioning. Please use ticket_number.n.diff
or ticket_number.n.patch
. It gives a better understanding.
Like here the name of your patch file could be 42238.2.patch
.
All patches usually use this convention.
Thank you!
#17
@
5 years ago
@subrataemfluence Thanks for addressing this. I will take care of these convention from now onwards. Unfortunately I read your comment after uploading a new patch. Will take care of this in future,
Thanks again!
#18
@
5 years ago
@TimothyBlynJacobs Thank you for guiding me the more appropriate way. I have made the changes and created a new patch
https://core.trac.wordpress.org/attachment/ticket/42238/42238-updated.patch
When you have time can you please verify whether the same works for you as well?
Yes, I am interested in writing a unit test to test this fix. Will provide it soon, probably by tomorrow. Is that ok?
Thanks again!
#19
@
5 years ago
@TimothyBlynJacobs I have added a patch file for Unit test as well https://core.trac.wordpress.org/attachment/ticket/42238/42238-unittest.patch.
Awaiting feedback from you,
Thanks!
#20
@
5 years ago
- Keywords has-unit-tests added; needs-unit-tests removed
- Milestone changed from Future Release to 5.4
Thanks for adding the unit test @imani3011!
I uploaded a patch combining your two patches into one and made some code style fixes
Instead of checking for a 200 response on the collection, I changed the test to specifically test for a 403 response. That way we are sure the comment isn't being inadvertently exposed.
Hi @dragosh635 welcome to trac and thanks for the ticket.
Here is how I tried to reproduced your issue:
Step 1:
Step 2:
which is different from yours.
I tested with
WP_DEBUG
set to bothtrue
andfalse
separately. Either cases the above message stayed the same for me.Please note, I did not test with Woocommerce as it is a third party plugin and preferred to stay with WP core functionality for creating and registering above custom post type.
If you think I made any mistake in the above process please let me know exactly how I can reproduce the issue. However, I would always prefer not to use any third party plugin to test WordPress core functionality :)