Add paging to the EntityListBuilder

With the new entity query for configurables, this might be possible and might even make sense :)

Started to work on that.

This now uses entity query to recieve it's items, so using range() is pretty easy.
There are for sure still a lot of problems: How to inject stuff, should this handle a unique pager element ID? Should this patch take care about ALL the implementations in core which work different?

Let's see how much fails if there is a proper amount of items returned.

#4: drupal-1798332-4.patch queued for re-testing.

#4: drupal-1798332-4.patch queued for re-testing.

I am not sure whether it is actually worth to do it.

Shipping with a Node and User list controller that doesn't do paging seems absolutely crazy :)

However, I agree that for config entities, it doesn't make much sense, as it would solely be a UI thing, we have to load them anyway for the entity query.

But maybe we should only do this in a ContentEntityListBuilder by default?

for config entities [...] we have to load them anyway for the entity query.

Isn't that a detail that is internal to the chosen storage implementation?

For example, in case #2162161: Change DatabaseStorage to use XML instead of serialize is going to happen, then the serialized config entities could be filtered and queried through native document query support in the storage engine; i.e., a corresponding config entity query implementation would not actually have to load all the entities first.

Yes, maybe, *but* config storage and config entity query is not on the same implementation layer. Config entities use the Config API to deal with raw configuration data, they don't know which storage or serializer is used and they shouldn't.

I don't see a way to implement that without implementing completely new API's or violating the API layers there.

I also doubt that the performance gain of doing that query in the database will worse the considerably slower serialization that XML would imply?

This seems major. For instance, if a site turns views off, the node listing page doesn't have paging.

This is a reroll of #4 that gets paging working.

Soooo...adding a pager to a table removes the HTML ID from the markup apparently, thus the fails.

Actually, it isn't so much adding the pager, as moving the build array in one level to $build['table'] that strips the ID. Not sure if there is a known issue for that, or a workaround.

Nevermind, I realized the ID was manually being added, and the change mentioned in #19 moved the table so the ID wasn't properly added. This should fix some, if not all, of the fails.

This should address the remaining fails. I also added a wrapper method as suggested in #22 for getting the current page.

See #2136559: Config entity admin lists show configuration with overrides (eg. localized), we will need to update for that issue. As commented there, I suggest we move the entity query in a separate helper function, so it is easier to re-use and override.

I don't get why this is using range() and not pager(), which is always available and takes care of the paging for us? Should simplify things.

The patch in #26 is still using range() instead of pager(). It also has several coding standard issues.

Rerolled

Here is a reroll that should apply. It also addresses the pager part of #25, which did significantly simplify things!

See #2136559: Config entity admin lists show configuration with overrides (eg. localized), we will need to update for that issue. As commented there, I suggest we move the entity query in a separate helper function, so it is easier to re-use and override.

I don't fully understand what needs to be changed regarding this comment though.

The problem is that ConfigEntityListBuilder has overridden load() now, so config entities no longer call this.

So my suggestion is to move the entity query part into a protected function getEntityIds() or so, and then pass that to $storage->loadMultiple(), then you can call that as well in ConfigEntityListBuilder::load(), and we can use a pager for config entities too.

Thanks for the clarification Berdir, that makes sense. How's this approach look?

Yes, that is what I meant.

We probably need some test coverage for this, e.g. create 51 nodes and check that we have a pager? And because the code path is different, the same for a config entity. We should use a different one than NodeType there I think, because those are expensive to create. Maybe use ConfigTest (and EntityTest, although I'm not sure if we have a collection route for them right now).

Good call on tests.

In case there's any doubt, test also attached separately.

Oops, forgot to add config entity test. I'll work on that.

This adds a config entity test.

Updating issue summary and adding a beta phase evaluation.

Committed/pushed to 8.0.x, thanks!

Follow-up #2895320: Translated menu's name and description are not showing up under the target language.

List builder should load entities using overrides to reflect UI language