Files: 
CommentFileSizeAuthor
#14 2068343_13.patch4.44 KBslashrsm
PASSED: [[SimpleTest]]: [MySQL] 58,761 pass(es).
[ View ]
#14 interdiff.txt2.87 KBslashrsm
#12 2068343_12.patch2.89 KBslashrsm
PASSED: [[SimpleTest]]: [MySQL] 58,589 pass(es).
[ View ]
#12 interdiff.txt2.72 KBslashrsm
#7 2068343_7.patch1.22 KBslashrsm
PASSED: [[SimpleTest]]: [MySQL] 58,297 pass(es).
[ View ]
#7 interdiff.txt2.53 KBslashrsm
#4 2068343_4.patch2.99 KBslashrsm
FAILED: [[SimpleTest]]: [MySQL] 58,995 pass(es), 1 fail(s), and 1 exception(s).
[ View ]
#2 2068343_2.patch4.06 KBslashrsm
FAILED: [[SimpleTest]]: [MySQL] 58,191 pass(es), 45 fail(s), and 88 exception(s).
[ View ]

Comments

Assigned:Unassigned» slashrsm

Working on this.

Status:Active» Needs review
StatusFileSize
new4.06 KB
FAILED: [[SimpleTest]]: [MySQL] 58,191 pass(es), 45 fail(s), and 88 exception(s).
[ View ]

Go for it, testbot!

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new2.99 KB
FAILED: [[SimpleTest]]: [MySQL] 58,995 pass(es), 1 fail(s), and 1 exception(s).
[ View ]

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

  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.

Status:Needs work» Needs review
StatusFileSize
new2.53 KB
new1.22 KB
PASSED: [[SimpleTest]]: [MySQL] 58,297 pass(es).
[ View ]

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

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

Status:Needs review» Needs work

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

Status:Needs work» Needs review

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

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

Status:Needs work» Needs review
StatusFileSize
new2.72 KB
new2.89 KB
PASSED: [[SimpleTest]]: [MySQL] 58,589 pass(es).
[ View ]

Good point.

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.

Status:Needs work» Needs review
StatusFileSize
new2.87 KB
new4.44 KB
PASSED: [[SimpleTest]]: [MySQL] 58,761 pass(es).
[ View ]

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.

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.

Status:Reviewed & tested by the community» Fixed

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

YAY!

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