Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Element/Template/GetList processor for >20 templates in manager #16145

Closed
wants to merge 2 commits into from

Conversation

halftrainedharry
Copy link
Contributor

What does it do?

Currently the Element/Template/GetList processor always returns a 'total' value of 0. This causes the pagination of the template-dropdown to not work properly.

Also in the "Create Document" window, there is no pagination and the filtering happens without further AJAX-requests. Therefore the processor should return all templates if no limit is specified.
(Maybe a better fix would be to send a limit = 0 request parameter from the form though.)

Why is it needed?

If you have more than 20 templates, not all of them can be selected when creating/updating a resource in the manager.

How to test

Create more than 20 templates.
Create/update a resource and test if all templates can be selected.

Related issue(s)/PR(s)

Resolves #16135

@cla-bot cla-bot bot added the cla-signed CLA confirmed for contributors to this PR. label Apr 14, 2022
@Ibochkarev Ibochkarev added this to the v3.0.1 milestone Apr 14, 2022
@Ibochkarev Ibochkarev added the pr/review-needed Pull request requires review and testing. label Apr 14, 2022
Copy link
Collaborator

@smg6511 smg6511 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works as advertised!

@Jako
Copy link
Collaborator

Jako commented Apr 22, 2022

The limit line should not make any difference. limit = 0 is sent with the initial Element/Template/GetList request.

The sorting should be moved to an additional prepareQueryAfterCount method

    public function prepareQueryAfterCount(xPDOQuery $c)
    {
        $c = parent::prepareQueryAfterCount($c);
        $c->sortby('category_name');
        $c->sortby('templatename');

        return $c;
    }

for two reasons:

  • category_name is selected (created) in parent::prepareQueryAfterCount
  • sorting (normally) does not change the count and without sorting the count is less resource consuming.

@smg6511
Copy link
Collaborator

smg6511 commented Apr 22, 2022

@Jako - Unless the sorting (in it's current placement) is affecting the number of returned results, altering the limit might be necessary, as it's set to 20 in one of the ancestor classes (core/src/Revolution/Processors/Model/GetListProcessor.php). I see what you're saying about it being set to 0 in the baseParams in the MODx.combo.Template definition. Did you test what you posted above while removing the limit proposed in this PR?

@halftrainedharry
Copy link
Contributor Author

@Jako Element/Template/GetList is used in various places. At least in the "Create Document" window, there is no limit sent in the request. There may be other places.

Screenshot 2022-04-23 at 07-55-51 Dashboard MODX Revolution

@Jako
Copy link
Collaborator

Jako commented Apr 23, 2022

I have tested my comment and there was no issue with about 25 templates. It is somehow a BC if the default limit is changed. Setting the limit has to go in the JS code.

@halftrainedharry
Copy link
Contributor Author

Sure. Feel free to change this PR or create a new one.

@halftrainedharry
Copy link
Contributor Author

Closing in favor or #16160.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed CLA confirmed for contributors to this PR. pr/review-needed Pull request requires review and testing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MODX 3.0.0 Template Menu in Quick and regular Edit mode
4 participants