Simplify traversal

[thet]

by @petri
Opening this PR to be able to discuss it right here.

EDIT (petri) For background, see discussion in https://community.plone.org/t/json-application-messaging-in-plone-core/12353/11

[mister-roboto]

@thet thanks for creating this Pull Request and help improve Plone!

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass.

Whenever you feel that the pull request is ready to be tested, either start all jenkins jobs pull requests by yourself, or simply add a comment in this pull request stating:

@jenkins-plone-org please run jobs

With this simple comment all the jobs will be started automatically.

Happy hacking!

[thet]

@jenkins-plone-org please run jobs

[petri]

Should we check for ITraversable rather than or in addition to IContentish? Doing that seems to still solve plone/plone.restapi#680 and plone/plone.app.theming#165 (even when converted to IService!) whilst still passing at least plone.rest tests.

[tisto]

Interesting. I'd guess that IContentish is always ITraversable. IContentish excludes the SiteRoot, not sure if that is traversable. Things are complex and different people worked on different parts that depend on plone.rest, so I'd like them to weight in as well. cc @buchi @jaroel @sneridagh

[jaroel]

I actively remove IContentish from Plone Site in https://github.com/plone/Products.CMFPlone/pull/2612/files#diff-860754a57fed2533e4aae6aab34a27caR238-R242 . I did not check how that will affect plone.rest stuff, but so far it seemed to work.
My gut reaction is that ITraversable is correct here.

[jaroel]

We're adding IContentish back to PloneSite.
I don't think this changes my position on ITraversable, but it probably will affect other stuff.

Is this whole branch/approach still relevant actually?

[petri]

@jenkins-plone-org please run jobs

[jensens]

@jenkins-plone-org please run jobs

[petschki]

@jenkins-plone-org please run jobs

[petschki]

@jenkins-plone-org please run jobs

[petschki]

@jenkins-plone-org please run jobs

[petschki]

@jenkins-plone-org please run jobs

[tisto]

@thet @petri @petschki could you elaborate on the background of this simplification? What's the actual use case here and the reasoning behind this PR?

The traversal mechanism has been subject to multiple refactorings in the past and it needs to cover multiple edge cases that we can not always put into proper unit tests. Changing this requires careful consideration and extensive testing and review. I'd like to understand the reasoning behind this for a proper tradeoff of the risks and chances here.

[petschki]

I'll try my best...

OLD strategy:

Try Zope's DefaultPublishTraverse first an catch it's KeyError as trigger to check for REST service or BrowserView and if nothing found re-raise KeyError to go back to the DefaulPublishTraverse again. The downside of this is, that the special isinstance check for VirtualHostMonster (and possibly other checks) have to be included in plone.rest traversal.

NEW strategy:

1. Check for IService -> we can return this early
2. Go with the DefaultPublishTraverse -> this raises KeyError (eg. if traversed name not found) and follows the default zope NotFound traversal mechanism
3. Check for contentish objects -> wrap them with the RESTWrapper
4. Last return object if it's something we do not cover (eg. VirtualHostMonster)

So this does exactly the same as before with the benefit of getting rid of the VirtualHostMonster (and possibly other objects) check. If there are missing special traversal objects/paths, they were missing before too. But they can be easily extended after step 3. and before step 4.

hope this helps.

[petschki]

NOTE: this branch is now on a Plone4.3/collective.exportimport migration project of us and works like a charm. The linage subsites delivered with VirtualHostMonster returns now plone.restapi JSON responses. Before there was this error:

"Resource not found: https://www.klaus.at/Plone/klaus/virtual_hosting"

/cc @thet I think you also had this one?

[petschki]

btw. the Jenkins jobs are flapping randomly ... yesterday it was https://jenkins.plone.org/job/pull-request-5.2-3.8/ today it's https://jenkins.plone.org/job/pull-request-5.2-2.7/ but I didn't change anything ... has anybody a clue on this?

/cc @jensens @rodfersou

[tisto]

Thanks for the clarification @petschki.

@buchi @sneridagh could you have a look at this PR?

[petschki]

@jenkins-plone-org please run jobs

[petri]

If someone wants to get some more background on this PR, I added a link to the discussion (see thet's original post at the beginning of this PR). It in turn has pointers to some earlier discussions. One thing worth mentioning is that the PR should also let plone.app.theming UI work properly again... afaik it has been partially non-functional ever since plone.rest was included with plone core.

[petri]

@jenkins-plone-org please run jobs