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

[4] webservices consistency #34093

Merged
merged 10 commits into from
Jun 1, 2021
Merged

[4] webservices consistency #34093

merged 10 commits into from
Jun 1, 2021

Conversation

alikon
Copy link
Contributor

@alikon alikon commented May 22, 2021

Pull Request for discussion #34080 (comment)

Summary of Changes

so
v1/content/article became v1/content/articles

v1/content/article/contenthistory/ became v1/content/articles/contenthistory/
v1/content/article/contenthistory/keep became v1/content/articles/contenthistory/keep

v1/contact/form/:id became v1/contacts/form/:id
v1/contact became v1/contacts
v1/contact/categories became v1/contacts/categories

v1/fields/contact/contact became v1/fields/contacts/contact
v1/fields/contact/mail became v1/fields/contacts/mail
v1/fields/groups/contact/contact became v1/fields/groups/contacts/contact
v1/fields/groups/contact/mail became v1/fields/groups/contacts/mail
v1/fields/groups/contact/categories became v1/fields/groups/contacts/categories

v1/contact/contenthistory/:id became v1/contacts/contenthistory/:id
v1/contact/contenthistory/keep/:id became v1/contacts/contenthistory/keep/:id

v1/redirect became v1/redirects

v1/privacy/request became v1/privacy/requests
v1/privacy/request/export/:id became v1/privacy/requests/export/:id

v1/privacy/consent became v1/privacy/consents

Testing Instructions

call the all the above endpoints

Documentation Changes Required

yes

@richard67
Copy link
Member

@alikon I think you will have to adapt the API tests to that because they are failing in Drone: https://ci.joomla.org/joomla/joomla-cms/44217/1/25

@richard67
Copy link
Member

@alikon A quick search for v1/content/article on a 4.0-dev branch shows 1 more place which needs to be changed:

https://github.com/joomla/joomla-cms/blob/4.0-dev/api/components/com_content/src/Serializer/ContentSerializer.php#L46

@richard67
Copy link
Member

richard67 commented May 22, 2021

@alikon And looking for /content/articleshows 3 more places in tests to be changed:

./tests/Codeception/api/com_content/ContentCest.php:86:         $I->sendGET('/content/article/1');
./tests/Codeception/api/BasicCest.php:66:               $I->sendGET('/content/article/1');
./tests/Codeception/api/BasicCest.php:83:               $I->sendGET('/content/article/1');

@alikon
Copy link
Contributor Author

alikon commented May 22, 2021

thanks @richard67 one of these day maybe i'll need to set codeception 😃

@richard67
Copy link
Member

richard67 commented May 22, 2021

There is nothing more reliable than a search on command line.

On Linux: find ./ -type f -name "*\.php" -exec grep -Hn "/content/article" {} \;

On Windows: findstr /s /c "/content/article" *.php

@richard67
Copy link
Member

@alikon I was just reading the docs https://docs.joomla.org/J4.x:Joomla_Core_APIs to see what needs to be changed, and I saw that there is still singular used for "Contact":
curl -X GET /api/index.php/v1/contact

Is the docs outdated?

If not, then this should be changed, too, for consistency.

@richard67
Copy link
Member

@alikon Beside the contacts, that doc shows 3 more where singular is used:

curl -X GET /api/index.php/v1/privacy/request
curl -X GET /api/index.php/v1/privacy/consent
curl -X GET /api/index.php/v1/redirect

@alikon
Copy link
Contributor Author

alikon commented May 22, 2021

if we will get green light on this....i'll do the other changes...i don't want to spend time on endless discussions
as twitter said there will be an RC on tuesday.... 🤣

@richard67 richard67 added the RMDQ ReleaseManagerDecisionQueue label May 22, 2021
@richard67
Copy link
Member

@wilsonge Do you want that to be done, what this PR here does plus my findings above, before RC?

@joomdonation
Copy link
Contributor

If this is accepted, we should do the same for contact (which is using singular at the moment). Also, maybe a stupid question: Why do we need v1/content/articles, not just v1/articles like v1/banners....

@richard67
Copy link
Member

If this is accepted, we should do the same for contact (which is using singular at the moment). Also, maybe a stupid question: Why do we need v1/content/articles, not just v1/articles like v1/banners....

@joomdonation If you scroll a little bit up you will find 1 comment by me listing more than not just contact being singular: #34093 (comment) , just below my comment where I already reported that for contact.

@joomdonation
Copy link
Contributor

Ah Yes @richard67. See it now !

@wilsonge
Copy link
Contributor

Happy to move to plural. As I said in the issue the only thing is that the “type” property in the API body (contentType property in the view) must be singular (by the json api standard)

@pjdevries
Copy link
Contributor

@alikon Can this be tested now?

@alikon
Copy link
Contributor Author

alikon commented May 26, 2021

yes only for article vs articles

@pjdevries
Copy link
Contributor

I'll wait until the others are implemented as well then.

@alikon alikon marked this pull request as draft May 27, 2021 06:42
@alikon alikon marked this pull request as ready for review May 27, 2021 06:57
@alikon
Copy link
Contributor Author

alikon commented May 27, 2021

com_contact, com_privacy, com_redirect covered now

@pjdevries
Copy link
Contributor

I made a start by testing the articles endpoints. The two GET's and POST work as expected. PATCH works but validation is not properly implemented (see #34261). DEL gives me a 500.

@alikon
Copy link
Contributor Author

alikon commented May 28, 2021

good catch 👍
api-tests is green..

probably already there ...
anyway i'll check it #34261

@pjdevries
Copy link
Contributor

api-tests is green..

Do you mean the PATCH test? I checked it and the reason it passes is because the request data of the test includes the title and catid fields. Those must be required for new articles but not for PATCH requests.

@alikon
Copy link
Contributor Author

alikon commented May 28, 2021

yes
outside the scope of this "silly" 1

@alikon
Copy link
Contributor Author

alikon commented May 29, 2021

DEL gives me a 500.

you can delete an article only if it is in trashed status (-2)

for reference see:

@pjdevries
Copy link
Contributor

DEL gives me a 500.

you can delete an article only if it is in trashed status (-2)

I very much agree with your reservations in #31130 about having to make 2 requests in order to delete an item. I have the distinct feeling a lot of effort has been put into using as much existing code as possible while implementing the web services. Although I am very much in favor of code reuse, I also think there are important differences between the regular front end services and the web services. This issue is an example of that, as well as the PATCH validation problem I mentioned earlier.

I also think a 500 is not the proper response code for the failure of such an action. Maybe 409 is more appropriate?

Anyway, I executed the PATCH & DELETE combo and then the article is deleted as it should.

@pjdevries
Copy link
Contributor

I tested the content/articles/contenthistory endpoints and these are my findings:

  • GET {{base_path}}/api/index.php/v1/content/articles/contenthistory/{{article_id}} gives me an empty result, even though there are several history items for the targeted article.
  • PATCH {{base_path}}/api/index.php/v1/content/articles/contenthistory/keep/{{article_id}} properly toggles keep_forever.
  • DELETE {{base_path}}/api/index.php/v1/content/articles/contenthistory/{{article_id}} only deletes the targeted history item if keep_forever is false. When truethe history item is not deleted, but no feed back is given in the response. Worse even, the response is the same in both cases:1`.

Some responses do not comply with the JSON:API specifications. From what I have seen, PATCH and DELETE don't send back proper JSON:API responses. The former responding with just [] and the latter with a literal 1 (see also #34269).

@wilsonge wilsonge merged commit 631589e into joomla:4.0-dev Jun 1, 2021
@wilsonge
Copy link
Contributor

wilsonge commented Jun 1, 2021

Thanks.

@pjdevries can you open an issue for the content history endpoints. If it's broken it needs fixing and risks getting lost now this is merged.

@wilsonge wilsonge added this to the Joomla 4.0 milestone Jun 1, 2021
@wilsonge wilsonge removed the RMDQ ReleaseManagerDecisionQueue label Jun 1, 2021
@alikon alikon deleted the patch-81 branch June 1, 2021 18:27
@pjdevries
Copy link
Contributor

@pjdevries can you open an issue for the content history endpoints. If it's broken it needs fixing and risks getting lost now this is merged.

See #34361

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.

6 participants