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

fix #1158, show on rest api only the fields of the post type #1238

Open
wants to merge 1 commit into
base: develop
from

Conversation

Projects
None yet
2 participants
@Mte90
Copy link

commented Mar 12, 2019

Description

Show on rest api only the fields of the specific post type instead right now that show all the registered fields in all the various metabox.

Basically the problem is that now when you define different metaboxes on various post types and open a rest api page and see the fields you can find all of them (also of other post types) as empty.
Checking the code the problem was that there is an array filled with all of them without a check if it is the right place to show them.

There are issues with the tests right now and need to be fixed before this patch is ready to be approved.

Motivation and Context

Fixes #1158.

Testing procedure

Create 2 metabox to 2 different post types and you will see only the fields of the post type thaty ou are looking on rest api (check in the ticket for an example of the code)

Checklist:

@jtsternberg

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

Thanks for the PR! Just a general word of recommendation regarding your PR summary. It is much more likely the maintainers will be able to review and test if you provide more robust testing instructions. E.g. if you could provide the CPT setup you are using, the CMB box configuration, and the exact routes where you would see X or Y, that will go a long way towards us being able to quickly review and test.

@Mte90

This comment has been minimized.

Copy link
Author

commented Mar 12, 2019

Seems that the bug in tests is that on https://github.com/CMB2/CMB2/blob/develop/tests/test-cmb-rest.php#L227 is creating a new REst_request that is not a string as suggesting by the comments in the code

@Mte90

This comment has been minimized.

Copy link
Author

commented Mar 12, 2019

The CMB example code is in the ticket that show the issue :-)

@jtsternberg

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

The CMB example code is in the ticket that show the issue :-)

I think maybe you're misunderstanding. I am not saying we won't be able to figure out how to test this. Of course we can figure out how to test it.

I'm saying the neater/nicer package you can give in your PR (even if you have to copy/paste from another issue) will go a long way towards getting the PR reviewed. I'm sure you're a busy guy too, and I very much appreciate the contribution, but when I see a PR with a description like this (where the description assumes context, where testing instructions are basically "do X", where the "Checklist" items are completely ignored/unchecked), it moves this PR to the bottom of my todo list, as I know it'll require extra hunting, and based on the PR description, that I won't get much help from the provider of the code, who does not appear to care much about the health of the project as much as caring that their piece of code gets added to deal with their one-off problem.

Maybe that's not you, but when a PR is submitted in this way, this is what it indicates.

As this project (and many others I maintain) are used and contributed to quite often, the more help I can get from the contributor, the more the workload is evened out among the many instead of the few.

@Mte90

This comment has been minimized.

Copy link
Author

commented Mar 12, 2019

Yes, sorry I was having connectivity issues and studying the tests issue while pushing that patch on a job project -.-'

I improved a bit the description and stuff in the ticket.

Can you help me on understanding the problem with the tests?

@jtsternberg

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

I can look at/fix the tests when I test this. Do me a favor and update the PR description to note that the tests will need to be fixed, so I don't forget. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.