plone / plone.restapi

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

@contextnavigation endpoint uses plone.api #1641

Closed erral closed 1 year ago

erral commented 1 year ago

The @contextnavigation endpoint uses plone.api in its implementation, which, if I am not wrong, is not allowed by our policy:

https://github.com/plone/plone.restapi/blob/master/src/plone/restapi/services/contextnavigation/get.py#L8

kshitiz305 commented 1 year ago

@erral HI I would like to contribute to it, may I know what should be the ideal API that needs to be used.

stevepiercy commented 1 year ago

@kshitiz305 please read and follow Contributing to Plone and First-time contributors. As far as what needs to be done, @erral may have more information.

kshitiz305 commented 1 year ago

@stevepiercy I have already gone through the agreement and would like to start contribution this would be my second contribution the project

stevepiercy commented 1 year ago

@kshitiz305 unless I am mistaken, you are not a member of the Plone GitHub organization, nor do you have any contributions to this project.

kshitiz305 commented 1 year ago

@stevepiercy I had sent the Signed agreement Once the agreement he is verified by your team the pull request will be merged https://github.com/plone/plone.app.contentmenu/pull/48

erral commented 1 year ago

The contextnavigation endpoint uses a plone.api.content.get_state call to get the content's state

https://github.com/plone/plone.restapi/blob/master/src/plone/restapi/services/contextnavigation/get.py#L401

But the use of plone.api is only allowed in tests of this plone.restapi package, so this call should be replaced with the proper call to the get the state of the content.

You can check some other parts of plone.restapi to find the proper method to be called.

davisagli commented 1 year ago

@erral plone.api is already used in a few places. I think it is no problem, since plone.restapi is considered a core addon that sits on top of CMFPlone. It does not create a circular dependency, as long as plone.api does not import from plone.restapi.

@jensens Did I get it right?

jensens commented 1 year ago

After our discussions at Beethoven sprint: this is correct. Be sure to not introduce performance issues, because plone.api adds some overhead with all its double checks. Usually it should be fine.