search

plone / plone.restapi

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

Improve the block transforms to include also anchors when resolving uid back and forth #1746

Closed sneridagh closed 5 months ago

sneridagh commented 5 months ago

Now it covers the use case if an anchor is present.

netlify[bot] commented 5 months ago

Deploy Preview for plone-restapi canceled.

Name Link
Latest commit 42df117cb6d3702843c8f34ea44d1f3bec8ea1f7
Latest deploy log https://app.netlify.com/sites/plone-restapi/deploys/65bc226c7ea4f6000840d3a3
mister-roboto commented 5 months ago

@sneridagh thanks for creating this Pull Request and helping to improve Plone!

TL;DR: Finish pushing changes, pass all other checks, then paste a comment:

@jenkins-plone-org please run jobs

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass, but it takes 30-60 min. Other CI checks are usually much faster and the Plone Jenkins resources are limited, so when done pushing changes and all other checks pass either start all Jenkins PR jobs yourself, or simply add the comment above in this PR to start all the jobs automatically.

Happy hacking!

sneridagh commented 5 months ago

@jenkins-plone-org please run jobs

sneridagh commented 5 months ago

I know, it’s one or the other, never both. Do you have a use case in mind?

Víctor Fernández de Alba Github/Twitter: sneridagh

On Wed, 31 Jan 2024 at 20:37, David Glick @.***> wrote:

@.**** requested changes on this pull request.

In src/plone/restapi/deserializer/utils.py https://github.com/plone/plone.restapi/pull/1746#discussion_r1473371669:

@@ -25,11 +25,13 @@ def path2uid(context, link): )

 # handle edge-case when we have non traversable path like /@@download/file
  • if "/@@" in path:
  • path, suffix = path.split("/@@", 1)
  • suffix = "/@@" + suffix
  • else:
  • suffix = ""
  • SUFFIXES = ["/@@", "#"]
  • suffix = ""
  • for suffix_separator in SUFFIXES:
  • if suffix_separator in path:
  • path, suffix = path.split(suffix_separator, 1)

@sneridagh https://github.com/sneridagh You are overwriting suffix here. So if both separators are present, only one of the suffixes will be kept.

In src/plone/restapi/serializer/utils.py https://github.com/plone/plone.restapi/pull/1746#discussion_r1473372777:

@@ -23,13 +23,15 @@ def resolve_uid(path): if match is None: return path, None

  • uid, suffix = match.groups()
  • uid, suffix, anchor = match.groups() brain = uuidToCatalogBrain(uid) if brain is None: return path, None href = brain.getURL() if suffix: return href + "/" + suffix, brain

What if there is a suffix and an anchor?

— Reply to this email directly, view it on GitHub https://github.com/plone/plone.restapi/pull/1746#pullrequestreview-1854675752, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADW4D3TZMLO7PPU4CO3EILYRKMOPAVCNFSM6AAAAABCQ2L5ECVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQNJUGY3TKNZVGI . You are receiving this because you were mentioned.Message ID: <plone/plone .@.***>

sneridagh commented 5 months ago

@davisagli I think not, because a suffix is something that matches a /@@ BrowserView. A BrowserView with an anchor, in RESTAPI world? Could be in ClassicLand, but not in RESTAPI. We could make it work, but I think that without a use case, it's premature optimization, could be that there's never a use case for that.

davisagli commented 5 months ago

@sneridagh Although this is plone.restapi, the URLs that an editor can enter are not API URLs. It could be a browser view that renders HTML including anchors, sure. Here we need to be careful and pass through unchanged anything that is not the path to a content item.

davisagli commented 5 months ago

@jenkins-plone-org please run jobs