plone / Products.CMFPlone

The core of the Plone content management system
https://plone.org
GNU General Public License v2.0
246 stars 186 forks source link

folder_contents calls @@getVocabulary twice #3464

Closed wesleybl closed 2 years ago

wesleybl commented 2 years ago

BUG/PROBLEM REPORT (OR OTHER COMMON ISSUE)

What I did:

I accessed the folder_contents from the root of the portal:

http://localhost:8080/Plone/folder_contents

What I expect to happen:

I expect @@getVocabulary to be called only once

What actually happened:

He called the @@getVocabulary URL twice, with the same parameters. The URL called twice is:

http://localhost:8080/Plone/@@getVocabulary?name=plone.app.vocabularies.Catalog&query={%22criteria%22:[{%22i%22:%22path%22,%22o%22:%22plone.app.querystring.operation.string.path%22,%22v%22:%22/::1%22}],%22sort_on%22:%22getObjPositionInParent%22,%22sort_order%22:%22ascending%22}&batch={%22page%22:1,%22size%22:15}&attributes=[%22Title%22,%22path%22,%22getURL%22,%22getIcon%22,%22getMimeIcon%22,%22portal_type%22,%22CreationDate%22,%22Creator%22,%22Description%22,%22EffectiveDate%22,%22ExpirationDate%22,%22ModificationDate%22,%22Subject%22,%22Type%22,%22UID%22,%22end%22,%22exclude_from_nav%22,%22getObjSize%22,%22id%22,%22is_folderish%22,%22location%22,%22review_state%22,%22start%22,%22mime_type%22,%22total_comments%22,%22last_comment_date%22]&_=1648307679950

Is this expected? Why is it called twice? For performance reasons, this is bad.

What version of Plone/ Addons I am using:

Plone 5.2 master and Plone 6 master

wesleybl commented 2 years ago

@thet Is this expected? could it improve?

wesleybl commented 2 years ago

@fredvd this might look like what you fixed in https://github.com/plone/mockup/pull/1031?

fredvd commented 2 years ago

@wesleybl I tried to fix a number of double requests together with @thet: and we managed to fix most of them. IIRC there are one or 2 conditions left where another component triggers fetching the same info again (but not always if you actually use the filter). :-(

I spend quite a few hours trying to debug the events firing and seeint which component fired them but it is very tricky to follow. With Plone 6.0 coming up I think most effort will be in that direction and iirc the folder_contents app is also partly rewritten for the es6 modules support.

The double request is sometimes inefficient, but it works.

wesleybl commented 2 years ago

With Plone 6.0 coming up I think most effort will be in that direction and iirc the folder_contents app is also partly rewritten for the es6 modules support.

The problem also occurs in the Plone 6 master.

petschki commented 2 years ago

@wesleybl @fredvd @thet during pat-structure refactorings for mockup#ES6 I came accross a fix for this. See https://github.com/plone/mockup/pull/1152

fredvd commented 2 years ago

Cool @petschki , thanks !! I circled around this issue for a few hours last year but couldn't find the exact trigger back then, also by looking through the call stack in the browser.

So the one line that updated the per page value with:

this.collection.howManyPer(perPage);

was causing the double result/collection refresh?

petschki commented 2 years ago

Exactly ... here https://github.com/backbone-paginator/backbone.paginator/blob/legacy/dist/backbone.paginator.js#L181 after setting perPage the complete pager() is called which fetches the results. So in our case its enough to set the perPage attribute on initialization and leave the rest up to the paginator API.

I came across this when trying to update backbone.paginator to version 2.0.4 which got a slightly changed and simplified API. I'll try to come up with a PR for master branch the next week.