search

plone / plone.restapi

RESTful API for Plone.
http://plonerestapi.readthedocs.org/
84 stars 75 forks source link

Undeprecate token parameter from vocabularies endpoint #1697

Closed tisto closed 1 year ago

tisto commented 1 year ago

Passing a token parameter to the vocabularies endpoint has been deprecated:

https://github.com/plone/plone.restapi/blob/eac02a701a42fd5853f9ec4b62d2d17dec283186/src/plone/restapi/serializer/vocabularies.py#L46

One should use "tokens" instead.

tisto commented 1 year ago

@sneridagh @davisagli I am unsure if we can remove allowing passing "token" instead of "tokens" parameter. Can you folks check if we do this properly in Volto 15, 16 and 17?

tisto commented 1 year ago

@sneridagh @davisagli does this mean that Volto currently sends both "token" and "tokens"?

https://github.com/plone/volto/blob/e91c4f95efe66c05c399bdc7a9d81987c245198d/src/actions/vocabularies/vocabularies.js#L73

davisagli commented 1 year ago

@tisto That particular code just passes through what was passed in to the request (token or tokens or both). You have to look at where that redux action is used

tisto commented 1 year ago

@sneridagh could you have a look at what we currently use in Volto 16 and 17? token or tokens? Dependent on what we use I'd make a decision on what to do...

sneridagh commented 1 year ago

@tisto we allow both, for backwards compat.

https://github.com/plone/volto/blob/b703425a07b4830bcdb58fbef8f0f25a0ddf5489/src/actions/vocabularies/vocabularies.js#L65

I don't know if we should remove the dual code from Volto as well... I'd leave it.

tisto commented 1 year ago

@sneridagh I'd say we remove "token" from plone.restapi 9 and deprecate it in Volto 17. We can then remove it in Volto 18. Otherwise, we risk incompatibilities between Volto and plone.restapi that are unnecessary.

davisagli commented 1 year ago

@sneridagh @tisto Currently volto can use either token or tokens, depending on whether the value is an array or not. I would personally prefer to keep supporting both in plone.restapi. I don't see a compelling reason to remove one of them. It would break compatibility of plone.restapi 10 with volto <17, and just make extra work for ourselves.

sneridagh commented 1 year ago

I also share the same feeling as David... it does not hurt, we document/use in code (the locations are also very localized, meaning they are not spreaded everywhere) just one.

davisagli commented 1 year ago

Done in #1708