? ? ? ? ? Pending

User tests: Successful: Unsuccessful:

avatar alikon
alikon
22 May 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

avatar alikon alikon - open - 22 May 2021
avatar alikon alikon - change - 22 May 2021
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 22 May 2021
Category Front End Plugins
avatar richard67
richard67 - comment - 22 May 2021

@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

avatar alikon alikon - change - 22 May 2021
Labels Added: ? ?
avatar joomla-cms-bot joomla-cms-bot - change - 22 May 2021
Category Front End Plugins Front End Plugins Unit Tests
avatar richard67
richard67 - comment - 22 May 2021

@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

avatar alikon alikon - change - 22 May 2021
Labels Added: ?
avatar richard67
richard67 - comment - 22 May 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');
avatar alikon
alikon - comment - 22 May 2021

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

avatar richard67
richard67 - comment - 22 May 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

avatar richard67
richard67 - comment - 22 May 2021

@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.

avatar richard67
richard67 - comment - 22 May 2021

@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

avatar alikon
alikon - comment - 22 May 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.... ?

avatar richard67
richard67 - comment - 22 May 2021

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

avatar joomdonation
joomdonation - comment - 23 May 2021

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....

avatar richard67
richard67 - comment - 23 May 2021

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.

avatar joomdonation
joomdonation - comment - 23 May 2021

Ah Yes @richard67. See it now !

avatar wilsonge
wilsonge - comment - 24 May 2021

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)

avatar pjdevries
pjdevries - comment - 26 May 2021

@alikon Can this be tested now?

avatar alikon
alikon - comment - 26 May 2021

yes only for article vs articles

avatar pjdevries
pjdevries - comment - 26 May 2021

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

avatar alikon alikon - change - 27 May 2021
Labels Added: ? ?
Removed: ?
avatar alikon alikon - change - 27 May 2021
The description was changed
avatar alikon alikon - edited - 27 May 2021
avatar alikon
alikon - comment - 27 May 2021

com_contact, com_privacy, com_redirect covered now

avatar alikon alikon - change - 27 May 2021
Labels Added: ?
Removed: ?
avatar alikon alikon - change - 27 May 2021
Labels Added: ?
Removed: ?
avatar pjdevries
pjdevries - comment - 28 May 2021

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.

avatar alikon
alikon - comment - 28 May 2021

good catch ?
api-tests is green..

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

avatar pjdevries
pjdevries - comment - 28 May 2021

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.

avatar alikon
alikon - comment - 28 May 2021

yes
outside the scope of this "silly" 1

avatar alikon
alikon - comment - 29 May 2021

DEL gives me a 500.

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

for reference see:

avatar pjdevries
pjdevries - comment - 29 May 2021

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.

avatar pjdevries
pjdevries - comment - 29 May 2021

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).

avatar wilsonge wilsonge - close - 1 Jun 2021
avatar wilsonge wilsonge - merge - 1 Jun 2021
avatar wilsonge wilsonge - change - 1 Jun 2021
Status Pending Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2021-06-01 15:20:57
Closed_By wilsonge
Labels Added: ? ?
Removed: ?
avatar wilsonge
wilsonge - comment - 1 Jun 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.

avatar pjdevries
pjdevries - comment - 2 Jun 2021

@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

Add a Comment

Login with GitHub to post a comment