Create default pager for the entity browser widgets

1. +++ b/src/Element/EntityBrowserPagerElement.php
   @@ -0,0 +1,80 @@
   +    $element['#attributes']['type'] = 'entity_browser_Pager';
   

   It should be all lowercase. "type" doesn't sound like a standard attribute. Why this decision?

2. +++ b/src/EntityBrowserPagerTrait.php
   @@ -0,0 +1,37 @@
   +/**
   + * Provides helper methods for entity browser pager.
   + */
   

   Should extensively document why this needs to exists and how is to be used.

3. +++ b/src/EntityBrowserPagerTrait.php
   @@ -0,0 +1,37 @@
   +  public function getCurrentPage(FormStateInterface $form_state) {
   

   This function needs to be called on every submit or the page number potentially won't be correct.

   Could we update page number ob a submit handler on the element itself?

1. +++ b/src/Element/EntityBrowserPagerElement.php
   @@ -0,0 +1,140 @@
   + * Provides an Entity Browser pager form element.
   + *
   

   Document how this element is used on a custom widget (examples, ...).

2. +++ b/src/Element/EntityBrowserPagerElement.php
   @@ -0,0 +1,140 @@
   + * - #paginate: Flag to indicate that pagination of entities will be done on
   + *   form element. If entities already paginated set this flag to FALSE.
   

   I'd default this to false.

3. +++ b/src/Element/EntityBrowserPagerElement.php
   @@ -0,0 +1,140 @@
   +    return [
   +      '#process' => [[$class, 'processEntityBrowserPager']],
   +      '#pre_render' => [[$class, 'preRenderEntityBrowserPager']],
   +      '#theme_wrappers' => ['form_element'],
   +      '#attached' => [
   +        'library' => ['entity_browser/pager'],
   +      ],
   

   Defaults are missing for values that are documented in the class comment.

4. +++ b/src/Element/EntityBrowserPagerElement.php
   @@ -0,0 +1,140 @@
   +    $page = $form_state->get('page') ?: 1;
   ...
   +    $page = $form_state->get('page') ?: 1;
   

   What happens when page was not saved to the form state yet?

5. +++ b/src/Element/EntityBrowserPagerElement.php
   @@ -0,0 +1,140 @@
   +    $element['previous'] = [
   +      '#type' => 'submit',
   ...
   +    $element['next'] = [
   +      '#type' => 'submit',
   

   You most likely want to use #limit_validation_errors on this.

6. +++ b/src/Element/EntityBrowserPagerElement.php
   @@ -0,0 +1,140 @@
   +      '#disabled' => count($entities) < $element['#per_page'],
   

   It might not always be possible to provide full list of entities (huge dataset, performance, ...). How do we figure out last page in that case?

7. +++ b/src/Element/EntityBrowserPagerElement.php
   @@ -0,0 +1,140 @@
   +    Element::setAttributes($element, [
   +      'entities_list',
   +      'per_page',
   +      'paginate',
   +    ]);
   +
   +    return $element;
   

   Where are this attributes used?

8. +++ b/src/Element/EntityBrowserPagerElement.php
   @@ -0,0 +1,140 @@
   +  public static function paginateEntities($entities_list, $page, $per_page = self::PER_PAGE) {
   +    return array_slice($entities_list, ($page - 1) * $per_page, $per_page);
   +  }
   

   Ok... now I understand. We need #entities_list just for figuring out last page number.

   In cases with huge datasets we'll pay big performance price for that. I'd rather rely on the implementing form to pass in the total number of pages (optional).

9. +++ b/tests/modules/entity_browser_test/src/Plugin/EntityBrowser/Widget/PagerTestWidget.php
   @@ -0,0 +1,83 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getForm(array &$original_form, FormStateInterface $form_state, array $additional_widget_parameters) {
   +    $form = parent::getForm($original_form, $form_state, $additional_widget_parameters);
   +    if (!$entities_list = \Drupal::entityTypeManager()->getStorage('node')->loadMultiple()) {
   +      $node_type = NodeType::create([
   +        'type' => 'example',
   +      ]);
   +      $node_type->save();
   +
   +      for ($i = 0; $i < 6; $i++) {
   +        /** @var \Drupal\node\NodeInterface $node */
   +        $node = Node::create([
   +          'type' => 'example',
   +          'title' => 'Test title',
   +          'langcode' => 'en',
   +        ]);
   +        $node->save();
   +      }
   +      $entities_list = \Drupal::entityTypeManager()->getStorage('node')->loadMultiple();
   +    }
   +
   +    $form['pager_eb'] = [
   +      '#type' => 'entity_browser_pager',
   +      '#per_page' => $this->configuration['items_per_page'],
   +      '#entities_list' => $entities_list,
   +      '#paginate' => TRUE,
   +    ];
   +
   +    return $form;
   +  }
   

   This could be simplified a lot. I'd only put pager on the widget and display message with the number of the current page on each page load.

   Then you can click pager buttons and check if the reported page seems correct + other things (buttons are disabled, ...)

10. +++ b/tests/src/FunctionalJavascript/PagerElementFormTest.php
    @@ -0,0 +1,105 @@
    +    $this->drupalGet('node/add/foo');
    

    If you go on /entity-browser/modal/pager instead you don't need JS (which will make test a bit faster).

1. +++ b/src/Element/EntityBrowserPagerElement.php
   @@ -0,0 +1,144 @@
   +      '#default_value' => [
   +        '#paginate' => FALSE,
   +        '#per_page' => static::PER_PAGE,
   +        '#entities_list' => [],
   +        '#total_pages' => FALSE,
   +      ],
   

   This is not correct. Those are all standalone values and have nothing to do with the #default_value one.

2. +++ b/src/Element/EntityBrowserPagerElement.php
   @@ -0,0 +1,144 @@
   +    $paginate = isset($element['#paginate']) ? $element['#paginate'] : $element['#default_value']['#paginate'];
   +    $per_page = isset($element['#per_page']) ? $element['#per_page'] : $element['#default_value']['#per_page'];
   +    $entities_list = isset($element['#entities_list']) ? $element['#entities_list'] : $element['#default_value']['#entities_list'];
   

   If you correctly declare default values for those 3 in getInfo() you don't need to do that.

3. +++ b/src/Element/EntityBrowserPagerElement.php
   @@ -0,0 +1,144 @@
   +    if ($paginate) {
   +      $entities_list = static::paginateEntities($entities_list, $page, $per_page);
   +    }
   

   I would get rid of #entities_list entirely. As mentioned in my previous review it can cause huge performance problems.

4. +++ b/src/Element/EntityBrowserPagerElement.php
   @@ -0,0 +1,144 @@
   +      '#limit_validation_errors' => [['previous']],
   ...
   +      '#limit_validation_errors' => [['next']],
   

   You will need to respect #parents here. It is generally possible that the pager appears deeper than top level in the form structure.

5. +++ b/tests/modules/entity_browser_test/src/Plugin/EntityBrowser/Widget/PagerTestWidget.php
   @@ -0,0 +1,62 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function defaultConfiguration() {
   +    return ['items_per_page' => 2] + parent::defaultConfiguration();
   +  }
   

   This configuration value is not used any more. Can be deleted.

1. +++ b/src/Element/EntityBrowserPagerElement.php
   @@ -0,0 +1,102 @@
   + * - #per_page: Number of items to be displayed per page.
   ...
   + *     '#per_page' => 8,
   ...
   +      '#per_page' => static::PER_PAGE,
   

   This seems not to be used any more.

2. +++ b/src/Element/EntityBrowserPagerElement.php
   @@ -0,0 +1,102 @@
   + * - #total_pages: Total number of pages.
   ...
   +      '#total_pages' => FALSE,
   ...
   +      '#disabled' => $element['#total_pages'] == $page,
   

   We need to explain that this is optional and what happens if it is not defined.

   Also $element['#total_pages'] == $page will be true for the default value and if $page is 0. I think that we need strict comparison here just in case.

   FALSE is not a very common pick for "undefined" which is what we essentially mean here if we don't provide a value. NULL is much more common in my opinion.

3. +++ b/src/Element/EntityBrowserPagerElement.php
   @@ -0,0 +1,102 @@
   + * Example:
   + *   $form['pager'] = [
   + *     '#type' => 'entity_browser_pager',
   + *     '#per_page' => 8,
   + *     '#total_pages' => 12,
   + *   ];
   

   How do I, as a developer that is using it, find out which page I am on?

4. +++ b/src/Element/EntityBrowserPagerElement.php
   @@ -0,0 +1,102 @@
   +  /**
   +   * Default entities per page.
   +   */
   +  const PER_PAGE = 10;
   

   I agree with @primsi that we should give this constant a better name.

Last nitpiks:

1. +++ b/src/Element/EntityBrowserPagerElement.php
   @@ -0,0 +1,114 @@
   + * - #per_page: Number of items to be displayed per page.
   

   Leftover.

2. +++ b/src/Element/EntityBrowserPagerElement.php
   @@ -0,0 +1,114 @@
   + * Example:
   + *   $form['pager'] = [
   + *     '#type' => 'entity_browser_pager',
   + *     '#total_pages' => 12,
   + *   ];
   

   Wrap in @code.

3. +++ b/src/Element/EntityBrowserPagerElement.php
   @@ -0,0 +1,114 @@
   + * by getCurrentPage($form_state) method from this element.
   

   Use @see

Fixed patch so it applies and improved docs a bit.

Added another helper function that resets the pager.

Fixed a typo in setCurrentPage() and improved tests to cover that case. I screwed the interdiff. Sorry.

Re-added test that escaped because of the rename.