Support from Acquia helps fund testing for Drupal Acquia logo

Comments

slashrsm’s picture

Assigned: Unassigned » slashrsm

Working on this.

slashrsm’s picture

Status: Active » Needs review
FileSize
4.06 KB

Go for it, testbot!

Status: Needs review » Needs work

The last submitted patch, 2068343_2.patch, failed testing.

slashrsm’s picture

Status: Needs work » Needs review
FileSize
2.99 KB

Let's try what happens w/o change in function system_update_8024().

twistor’s picture

  1. +++ b/core/modules/file/lib/Drupal/file/Plugin/views/argument/Fid.php
    @@ -23,13 +24,14 @@ class Fid extends Numeric {
    +    $titles = array();
    +    foreach ($fids as &$fid) {
    +      $file = $controller->load($fid);
    

    No need to have $fid be by reference.

  2. +++ b/core/modules/user/user.install
    @@ -636,22 +636,31 @@ function user_update_8011() {
    +    );
    +    if (empty($fid)) {
    +      $file = $controller->create($values);
    

    No need for empty(). Switch the conditions.

  3. +++ b/core/modules/file/lib/Drupal/file/Plugin/views/argument/Fid.php
    @@ -23,13 +24,14 @@ class Fid extends Numeric {
    +    $controller = \Drupal::entityManager()->getStorageController('file');
    +    $titles = array();
    +    foreach ($fids as &$fid) {
    +      $file = $controller->load($fid);
    +      $titles[] = String::checkPlain($file->getFilename());
         }
    

    These files should be bulk-loaded before the foreach. $controller->loadMultiple()

Can we convert update functions to entity query?

Edit: Leaving at NR for testbot.

Status: Needs review » Needs work

The last submitted patch, 2068343_4.patch, failed testing.

slashrsm’s picture

Status: Needs work » Needs review
FileSize
2.53 KB
1.22 KB

Fixed comments. chx confirmed on IRC that we do not touch update hooks, so I removed changes to user_update_8011().

slashrsm’s picture

#7: 2068343_7.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 2068343_7.patch, failed testing.

slashrsm’s picture

Status: Needs work » Needs review

#7: 2068343_7.patch queued for re-testing.

jibran’s picture

Status: Needs review » Needs work
Issue tags: +VDC
+++ b/core/modules/file/lib/Drupal/file/Plugin/views/argument/Fid.php
@@ -23,13 +24,14 @@ class Fid extends Numeric {
+    $controller = \Drupal::entityManager()->getStorageController('file');

This plugin needs injection @see Plugins can receive injected dependencies from the container

slashrsm’s picture

Status: Needs work » Needs review
FileSize
2.72 KB
2.89 KB

Good point.

dawehner’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/file/lib/Drupal/file/Plugin/views/argument/Fid.php
    @@ -17,19 +22,64 @@
    +  /**
    +   * Constructs a Drupal\Component\Plugin\PluginBase object.
    

    Seems to be just a copy and paste of some other code.

  2. +++ b/core/modules/file/lib/Drupal/file/Plugin/views/argument/Fid.php
    @@ -17,19 +22,64 @@
    +   * @param \Drupal\Core\Entity\EntityManager $entity_manager
    +   *   The entity manager.
    +   */
    +  public function __construct(array $configuration, $plugin_id, array $plugin_definition, EntityManager $entity_manager, QueryFactory $entityQuery) {
    

    You missed to document the QueryFactory.

  3. +++ b/core/modules/file/lib/Drupal/file/Plugin/views/argument/Fid.php
    @@ -17,19 +22,64 @@
    +    parent::__construct($configuration, $plugin_id, $plugin_definition);
    +    $this->entityManager = $entity_manager;
    +  }
    

    ... so we needs tests? This does NOT set $entityQuery so it should not work.

slashrsm’s picture

Status: Needs work » Needs review
FileSize
2.87 KB
4.44 KB

I'm ashamed... However, all comments from #13 should be fixed in attached patch. The only view in core that currently uses this argument plugin is file listing (detailed usage info part). I added some more tests to that, that should also catch non-working Fid plugin.

peximo’s picture

Status: Needs review » Reviewed & tested by the community

I have tested the patch and it works, also there's no coding standard problems; all seem good for me.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 0ff5e6d and pushed to 8.x. Thanks!

slashrsm’s picture

YAY!

Automatically closed -- issue fixed for 2 weeks with no activity.

plach’s picture