We could take the single - value fields and grep for them in the active directory in the relevant named files. If grep is not working then load 'em all and memory-query them, there's hardly anything else we can do to them.

This is almost a bug because right now there's no way without loading all the views to list them by a specific tag and that can be a serious regression if you happen to have a thousand or so Views.

Files: 
CommentFileSizeAuthor
#65 drupal-1846454-65.patch33.22 KBamateescu
PASSED: [[SimpleTest]]: [MySQL] 49,420 pass(es).
[ View ]
#65 interdiff.txt661 bytesamateescu
#63 drupal-1846454-63.patch32.58 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 49,573 pass(es).
[ View ]
#63 interdiff.txt6.49 KBdawehner
#61 drupal-1846454-61.patch31.5 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 49,416 pass(es).
[ View ]
#61 interdiff.txt2.26 KBdawehner
#60 1846454_60.patch31.32 KBchx
PASSED: [[SimpleTest]]: [MySQL] 49,388 pass(es).
[ View ]
#60 interdiff.txt12.75 KBchx
#59 1846454_59.patch30.85 KBchx
PASSED: [[SimpleTest]]: [MySQL] 49,369 pass(es).
[ View ]
#55 1846454_55.patch26.51 KBchx
PASSED: [[SimpleTest]]: [MySQL] 50,855 pass(es).
[ View ]
#55 interdiff.txt735 byteschx
#54 1846454_54.patch26.53 KBchx
PASSED: [[SimpleTest]]: [MySQL] 50,901 pass(es).
[ View ]
#54 interdiff.txt653 byteschx
#53 1846454_53.patch26.53 KBchx
PASSED: [[SimpleTest]]: [MySQL] 50,810 pass(es).
[ View ]
#51 1846454_51.patch24.48 KBchx
PASSED: [[SimpleTest]]: [MySQL] 50,852 pass(es).
[ View ]
#50 1846454_50.patch69.29 KBchx
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#48 3snfzi.jpg38.05 KBchx
#48 1846454_48.patch29.21 KBchx
FAILED: [[SimpleTest]]: [MySQL] 50,708 pass(es), 1 fail(s), and 13 exception(s).
[ View ]
#46 drupal-1846454-46.patch24.45 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 49,979 pass(es), 25 fail(s), and 82 exception(s).
[ View ]
#46 interdiff.txt4.52 KBdawehner
#45 drupal-1846454-45.patch23.66 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 50,313 pass(es), 29 fail(s), and 79 exception(s).
[ View ]
#45 interdiff.txt2.56 KBdawehner
#43 1846454_43.patch22.96 KBchx
FAILED: [[SimpleTest]]: [MySQL] 50,003 pass(es), 28 fail(s), and 80 exception(s).
[ View ]
#36 d7-views-list.png36.4 KBtim.plunkett
#28 drupal-1846454-27.patch18.94 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 49,470 pass(es), 45 fail(s), and 1,408 exception(s).
[ View ]
#26 1846454_25.patch11.87 KBchx
FAILED: [[SimpleTest]]: [MySQL] 48,080 pass(es), 64 fail(s), and 1,363 exception(s).
[ View ]
#25 1792536_25.patch26.15 KBchx
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1792536_25_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#24 1846454_24.patch6.5 KBchx
PASSED: [[SimpleTest]]: [MySQL] 50,625 pass(es).
[ View ]
#18 drupal-1846454-16.patch13.3 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1846454-16.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#15 interdiff.txt3.66 KBdawehner
#15 drupal-1846454-15.patch11 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 48,916 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
#9 1846454_9.patch10.57 KBchx
PASSED: [[SimpleTest]]: [MySQL] 48,751 pass(es).
[ View ]
#8 interdiff.txt4.68 KBdawehner
#6 grep_madness.patch5.46 KBchx
PASSED: [[SimpleTest]]: [MySQL] 48,322 pass(es).
[ View ]

Comments

I like the idea, because loading all configuration entities on listings is a problem in general but not a regression.

The question is whether we should fallback to memory query or tell people: Please install grep.
Another question, are we sure that using grep is a relyable way to query, if grep might be slightly different between installed versions/implementations (GNU vs. BSD).

We know the syntax in the active directory cos it is not hand edited but dumped by the storage driver. So we can and we will query extremely strictly on fixed strings. We will use fgrep. fgrep is available insanely widely.

Also, if someone else wants to do a research on OpenSolarisIndiana/NetBSD/FreeBSD/OpenBSD/GNU/hellknowswhatsavailableonwindows grep regexp support and add that, please do. I am mostly interested in adding support for fixed strings like tag not 'contains'. I can see the value in the latter but it's simply beyond what I have time for.

Hrm, discussing more it came to me we could do a test run like grep '^ 40*' config_get_config_directory() . '/system.site.yml' and if we get back 403/404 then we are golden.

We can use find findstr (newer doc page) on windows.

Status:Active» Needs review
StatusFileSize
new5.46 KB
PASSED: [[SimpleTest]]: [MySQL] 48,322 pass(es).
[ View ]

It's nowhere near ready but opinions are welcome. You can test with drush php-eval 'print_r(entity_query("view")->condition("name", "archive")->execute());'

Ps. yes. the value needs an escapeshellarg. I know. I will add one to the field as well.

StatusFileSize
new4.68 KB

Just post the tests.

StatusFileSize
new10.57 KB
PASSED: [[SimpleTest]]: [MySQL] 48,751 pass(es).
[ View ]

Thanks for the test but note that the result is an array of entity id => entity id :) so most of those tests pass except the OR because I havent coded that and the count query because I havent coded that either :D

Oh and use case for gazillions of configentities: one View per user. Yes you could do customization by saving only the values your customization UI allows but why not simply save the view itself?

Status:Needs review» Needs work

The last submitted patch, 1846454_9.patch, failed testing.

Status:Needs work» Needs review

That's an unrelated test failure, HEAD is broken atm: #1842726: Transliteration component must not contain drupal_alter().

#9: 1846454_9.patch queued for re-testing.

Seems like a very valuable feature, IMO. Would be OK if it worked only in limited use cases.

StatusFileSize
new11 KB
FAILED: [[SimpleTest]]: [MySQL] 48,916 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
new3.66 KB

Documents some parts, but no success to provide a way to list all configuration enties. git grep -L has a really odd behavior regarding the first item of the result.

Sounds audacious especially regarding Windows environment support, and potential highly restrictive shared hostings. Does anyone tested this with a proper WinNT install, Apache and IIS?

EDIT: Dawn I missed this https://drupal.org/node/1846454#comment-6758148 comment, anyway this doesn't seem to be implemented yet.

Status:Needs review» Needs work

The last submitted patch, drupal-1846454-15.patch, failed testing.

StatusFileSize
new13.3 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1846454-16.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Some work to get IN() support working.

Wouldn't it make sense to start with the least performant (and most awkward) implementation first? (i.e., listing and loading all files with a certain config entity prefix and checking certain properties against given values)

Anything OS/platform/tool/access/security-specific on top of that would be an optimization that may or may not be possible — but in any case, it wouldn't be reliable or guaranteed to exist and work.

In other words, an EFQ implementation for config entities won't buy us much if we cannot use it. :)

Thus, can we approach it in this order?

1) Slow, awkward, ugly, but guaranteed to work.
2) Use it.
3) Optimize it.

Sun's plan sounds good, first a platform independent implementation, then we can write platform specific implementations and enable them at install time after a environment check.

One advantage of this battle plan would be also, that you would first start with a full test coverage, as there is first full support and then replacing certain bits will ensure that it's still working.

hmmm. config is small, by number of files (in the thousands) and size.

why not shove them into a simple mysql table and use LIKE '%all the search terms%' ?

and it would be pluggable, so more sophisticated sites can use something better.

because it's absolutely impossible to react to config storage writes.

Status:Needs work» Needs review
StatusFileSize
new6.5 KB
PASSED: [[SimpleTest]]: [MySQL] 50,625 pass(es).
[ View ]

This is a new beginning based on the fantastic work beejeebus done: first came up with the idea of a denormalization table in #22 then in #1887244: use Config->save() and Config->delete() on import made it possible. This is not working yet -- the query engine is not written, the denormalization is.

Title:ConfigEntities could query with grepAdd Entity query to Config entities
StatusFileSize
new26.15 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1792536_25_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

It's slowly coming together.

StatusFileSize
new11.87 KB
FAILED: [[SimpleTest]]: [MySQL] 48,080 pass(es), 64 fail(s), and 1,363 exception(s).
[ View ]

That's the wrong patch.

Status:Needs review» Needs work

The last submitted patch, 1846454_25.patch, failed testing.

StatusFileSize
new18.94 KB
FAILED: [[SimpleTest]]: [MySQL] 49,470 pass(es), 45 fail(s), and 1,408 exception(s).
[ View ]

Just some small changes here and there, continue to work on.

Isn't this reintroducing something (potentially) like the active store we had prior to move to config cache? If I'm not mistaken we are writing more or less the same data in {config_entity_denormalized} and in {cache_config}, letting serialization alone.

Not sure where I'm going with this post, I was just a bit suprised to see this solution. I may be totally missing something.

Well config_entity_denormalized is all about enable to query against config, but cache_config
is all about caching for loading, so they fullfill two orthogonal use-cases and also the data-structure itself is simply not compatible.

We should certainl clear document that this table is NOT the actual storage.

also the data-structure itself is simply not compatible.

What I did not consider previously is that, if I understand correctly, the denormalization table holds one row per value, while cache_config holds one row per config object, correct?

@plach

Exactly that is the difference, they are fundamental different.

I'm wondering whether we want to require this table to be part of system module, but yeah there is probably no better place to do it.

Status:Needs work» Closed (won't fix)

Status:Closed (won't fix)» Needs work

Unilaterally wontfixing an issue is rude to other people following the discussion. One point of criticism in IRC is not reason to do so, either.

StatusFileSize
new36.4 KB

In D7 Views, we had the ability to filter and sort and search Views.
d7-views-list.png

In addition, there is EntityStorageControllerInterface::loadByProperties(). ConfigStorageController doesn't actually use that, but BlockStorageController does, but loading ALL the blocks, and foreaching over them. We should be moving that up to the parent anyway, since blocks aren't special, but that is horribly performant.

This would allow us to actually find and then load only the config entities we actually need. We talked about this as a "nice to have" in Paris, but discussed it seriously at the pre-BADCamp sprint.

In #19, sun suggested putting the grep idea on hold and going with a working implementation. That's in BlockStorageController in HEAD right now. This would be the next step.

I think config.module would be a reasonable place to put this.
Views UI already has some of its D7 functionality moved in there (sync, overridden) anyway.

This would be a life saver for the Field API CMI conversion, unless we come up with some sort of insane mechanism there that would be a mess to write and maintain, so if this is available just in the system (so without a module or whatever), + 1000. Ping me in case you feel this is workable so I can test this with that conversion.

So the decision is to move it to config so that the patch can be tested on the bot and then either sun's installer patch or PHP 5.3.10 happens (both of which makes the patch pass as it is) then we can discuss on moving it back to system for field's sake. It doesn't make a lot of sense to make it a separate module if the required field module depends on it.

There 's an entity module which was re-introduced for the entity displays, so maybe that's a better place ? It's required anyway and then we don't need to wait for either php 5.3.10 or the other patch.

That indeed sounds the best place!

Status:Needs work» Needs review
StatusFileSize
new22.96 KB
FAILED: [[SimpleTest]]: [MySQL] 50,003 pass(es), 28 fail(s), and 80 exception(s).
[ View ]

Let's take a look at this.

Status:Needs review» Needs work

The last submitted patch, 1846454_43.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.56 KB
new23.66 KB
FAILED: [[SimpleTest]]: [MySQL] 50,313 pass(es), 29 fail(s), and 79 exception(s).
[ View ]

Just added another test with nested conditions groups, though it fails at the moment.

At some point $conditionContainer is an instance of Condition not sql, can you please ensure that this fix is correct?

StatusFileSize
new4.52 KB
new24.45 KB
FAILED: [[SimpleTest]]: [MySQL] 49,979 pass(es), 25 fail(s), and 82 exception(s).
[ View ]

+++ b/core/modules/entity/lib/Drupal/entity/ConfigQuery/Denormalize.phpundefined
@@ -0,0 +1,117 @@
+  public function configSave(ConfigEvent $event) {
+    $transaction = $this->connection->startTransaction();
+    $this->configDelete($event);
+    if ($this->entityType && $this->connection->schema()->tableExists('config_entity_denormalized')) {
+      $this->insert = $this->connection->insert('config_entity_denormalized')
+        ->fields(array('entity_type', 'entity_id', 'name', 'value'));
+      $this->values($this->config->get());
+      $this->insert->execute();
+    }

It feels wrong that you can only get the entity type once something already called a configDelete() operator. Is that intended?

Fixed some minor documentation/codestyle things.

This interdiff is relative to #45

Status:Needs review» Needs work

The last submitted patch, drupal-1846454-46.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new29.21 KB
FAILED: [[SimpleTest]]: [MySQL] 50,708 pass(es), 1 fail(s), and 13 exception(s).
[ View ]
new38.05 KB

Status:Needs review» Needs work

The last submitted patch, 1846454_48.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new69.29 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Back to square one: by the decree of catch, we do not denormalize. So here's a complete in memory implementation, tested a "bit". Huge props to dawehner who wrote the sorting code and quite a few tests.

StatusFileSize
new24.48 KB
PASSED: [[SimpleTest]]: [MySQL] 50,852 pass(es).
[ View ]

Opsie, forgot to merge origin.

As I read through I was looking for real stuff to discuss, but all I found were nitpicks. Amazing work, and insanely awesome test coverage!

-----

+++ b/core/modules/entity/lib/Drupal/entity/ConfigQuery/Condition.phpundefined
@@ -0,0 +1,126 @@
+  public function compile($holder) {
+++ b/core/modules/entity/lib/Drupal/entity/ConfigQuery/Query.phpundefined
@@ -0,0 +1,55 @@
+    $holder = new \stdClass();
+    $holder->sort = $this->sort;

$holder seemed an odd name, and stdClass an odd choice. Apparently the interface calls it $query, and doesn't typehint it, so I guess its okay.

+++ b/core/modules/entity/lib/Drupal/entity/ConfigQuery/Condition.phpundefined
@@ -0,0 +1,126 @@
+    foreach ($this->conditions as $condition) {
+      if ($condition['field'] instanceOf ConditionInterface) {
+        $complex_conditions[] = $condition;
+      }
+      else {
+        $simple_conditions[] = $condition;
+      }

Maybe a comment here explaining the difference between simple and complex conditions? One implements an interface, the other...?

+++ b/core/modules/entity/lib/Drupal/entity/ConfigQuery/Condition.phpundefined
@@ -0,0 +1,126 @@
+        // Iterate over all conditions and act based on whether they are a condition
+        // group or a single condition.

Rewrap this to 80 chars

+++ b/core/modules/entity/lib/Drupal/entity/ConfigQuery/Condition.phpundefined
@@ -0,0 +1,126 @@
+            $match= $condition['operator'] === 'IS NULL';

missing a space after $match

+++ b/core/modules/entity/lib/Drupal/entity/ConfigQuery/Query.phpundefined
@@ -0,0 +1,55 @@
+      uasort($entities, function(&$a, &$b) use ($property, $direction) {

Why the & on $a and $b?

+++ b/core/modules/entity/lib/Drupal/entity/ConfigQuery/Query.phpundefined
@@ -0,0 +1,55 @@
+        $value_a = isset($a->{$property}) ? $a->{$property} : '';
+        $value_b = isset($b->{$property}) ? $b->{$property} : '';

These are entities, right? Can we do $value_a = $a->get($property); so that protected properties work?

+++ b/core/modules/entity/lib/Drupal/entity/ConfigQuery/Query.phpundefined
@@ -0,0 +1,55 @@
+    // Let the pager do it's work.

s/it's/its

+++ b/core/modules/entity/lib/Drupal/entity/ConfigQuery/Query.phpundefined
@@ -0,0 +1,55 @@
+    return drupal_map_assoc(array_keys($entities));
+  }
+}
diff --git a/core/modules/entity/lib/Drupal/entity/ConfigQuery/QueryFactory.php b/core/modules/entity/lib/Drupal/entity/ConfigQuery/QueryFactory.php
+++ b/core/modules/entity/lib/Drupal/entity/ConfigQuery/QueryFactory.phpundefined
@@ -0,0 +1,29 @@
+    return new Query($entity_type, $conjunction);
+  }
+}
diff --git a/core/modules/entity/lib/Drupal/entity/EntityBundle.php b/core/modules/entity/lib/Drupal/entity/EntityBundle.php
+++ b/core/modules/entity/lib/Drupal/entity/Tests/ConfigEntityQueryTest.phpundefined
@@ -0,0 +1,433 @@
+    $this->assertEqual(sort($result), $expected);
+  }

Missing blank line before end of the class

+++ b/core/modules/entity/lib/Drupal/entity/ConfigQuery/QueryFactory.phpundefined
@@ -0,0 +1,29 @@
+ * Factory class creating entity query objects for the config backend.

Should start with a verb, maybe "Provides" or "Defines"

+++ b/core/modules/entity/lib/Drupal/entity/ConfigQuery/QueryFactory.phpundefined
@@ -0,0 +1,29 @@
+   * Instantiate a entity query for a certain entity type.

Instantiates an entity query...

+++ b/core/modules/entity/lib/Drupal/entity/ConfigQuery/QueryFactory.phpundefined
@@ -0,0 +1,29 @@
+   *   A entity query for a specific configuration entity type.

An entity query

+++ b/core/modules/entity/lib/Drupal/entity/Tests/ConfigEntityQueryTest.phpundefined
@@ -0,0 +1,433 @@
+  static $modules = array('config_test');

public static, and needs a docblock

+++ b/core/modules/entity/lib/Drupal/entity/Tests/ConfigEntityQueryTest.phpundefined
@@ -0,0 +1,433 @@
+   * Stores all config entities created for that test.

the test, not that test

+++ b/core/modules/entity/lib/Drupal/entity/Tests/ConfigEntityQueryTest.phpundefined
@@ -0,0 +1,433 @@
+  protected $entities;
...
+    $this->entities = array();

Could move the = array() up to the property

+++ b/core/modules/entity/lib/Drupal/entity/Tests/ConfigEntityQueryTest.phpundefined
@@ -0,0 +1,433 @@
+    $this->kernelEnvironment = 'config_entity_query';

This doesn't have a declaration above

+++ b/core/modules/entity/lib/Drupal/entity/Tests/ConfigEntityQueryTest.phpundefined
@@ -0,0 +1,433 @@
+    $entity = entity_create('config_query_test', array(
+        'label' => $this->randomName(),
+        'id' => 1,
+        'number' => 31,
+      )

The indentation throughout is weird, most entity_create() calls just have )); on the last line and the values indented 2 spaces.

+++ b/core/modules/entity/lib/Drupal/entity/Tests/ConfigEntityQueryTest.phpundefined
@@ -0,0 +1,433 @@
+    $entity->save();
+  }
+
+
+  /**
+   * Test basic functionality.

Extra line

+++ b/core/modules/entity/lib/Drupal/entity/Tests/ConfigEntityQueryTest.phpundefined
@@ -0,0 +1,433 @@
+    $this->assertEqual($count, count($this->entities));

It probably doesn't matter, but you could s/assertEqual/assertIdentical throughout just to be 100% sure

+++ b/core/modules/entity/lib/Drupal/entity/Tests/ConfigEntityQueryTest.phpundefined
@@ -0,0 +1,433 @@
+   * @param $expected

@param array $expected

StatusFileSize
new26.53 KB
PASSED: [[SimpleTest]]: [MySQL] 50,810 pass(es).
[ View ]

Because of the double upload up there this might have slipped: huge props to dawehner for writing most of the tests and the base of the sort code.

Quite some cleanup: the superfactory is now ContainerAware and gets an EntityManager passed in which is then handed down to the backend specific factories via get. The field SQL backend will be able to get rid of entity_get_info once #1892462: EntityManager should use the CacheDecorator is in.

I moved the classes back to Core because as we do not SQL any more, we do not need a module space. The test moved to system.

Then I addressed almost all except "could move the = array() up to the property" -- that doesn't work because the class is not reinstantiated between test method runs and all the entities would accumulate in $entities.

assertIdentical was a great idea because it turned out the tests were broken (by me, Daniel doesn't make stupid mistakes like this), I was comparing the return value of sort (which is always TRUE) to the expected value and as we know TRUE == whatever almost always tends to be TRUE... There were, however, enough safeguards in place (in the form of counts) that the tests were *still* valueable. This now has been fixed and assertResults has been made significantly more resilient, the number of asserts climbed from 50 to 88 and now assertResults, true to its to own doxygen does not expect $expected to be sorted ahead of the time. At the same time, 30 LoC was deleted.

Adding an interdiff is entirely pointless so I skipped that.

StatusFileSize
new653 bytes
new26.53 KB
PASSED: [[SimpleTest]]: [MySQL] 50,901 pass(es).
[ View ]

Implemented "These are entities, right? Can we do $value_a = $a->get($property); so that protected properties work" for Condition as well.

StatusFileSize
new735 bytes
new26.51 KB
PASSED: [[SimpleTest]]: [MySQL] 50,855 pass(es).
[ View ]

Except the if beforehands needs to be converted too.

Status:Needs review» Needs work

The last submitted patch, 1846454_55.patch, failed testing.

So one general problem I don't know how to address, is that you can't use $config->get('display.default.display_options.path') yet as it is not a simple property.

Just to clarify on denormalization in case this comes back to bite me in a year:

I don't think it's worth denormalizing in the first implementation, that's something we can add later and even potentially backport from Drupal 9.

That only holds if usage of this API is restricted to cache misses and administrative listings. If code is running this in the critical path then it's doing something very wrong.

There might be valid use cases (mass updates?) where configuration objects of a particular type need to be loaded all at once and edited. If we're concerned about memory in this case we could use an iterator rather than loading everything at once into an array.

Status:Needs work» Needs review
StatusFileSize
new30.85 KB
PASSED: [[SimpleTest]]: [MySQL] 49,369 pass(es).
[ View ]

We have switched to using config arrays to address #57. As a bonus, we not just match display.default.display_options.path but display.*.display_options.path as well. I felt that'd be handy. The interdiff is, again, rather meaningless. We are reaching 100 asserts in this patch alone.

StatusFileSize
new12.75 KB
new31.32 KB
PASSED: [[SimpleTest]]: [MySQL] 49,388 pass(es).
[ View ]

This is mostly just tweaks , little better doxygen, test fixes and the like. I removed the single ->condition('foo') handling as that is most definitely premature optimization, it's now handled the same as ->condition('foo.bar'). Accidentally #59 didn't handle foo.bar although it did handle foo.*.bar this is now both fixed and tested. We now have 98 passes from 40 queries. I attached an interdiff -- note that match() didn't change at all, just moved.

StatusFileSize
new2.26 KB
new31.5 KB
PASSED: [[SimpleTest]]: [MySQL] 49,416 pass(es).
[ View ]

Some small cleanups.

This is RTBC but for a few missing docs and coding standards.
----

+++ b/core/lib/Drupal/Core/Config/Entity/Query/Condition.phpundefined
@@ -0,0 +1,171 @@
+   * Match for an array representing one or more config paths.

Matches

+++ b/core/lib/Drupal/Core/Config/Entity/Query/Condition.phpundefined
@@ -0,0 +1,171 @@
+   * @param $condition
+   *   The condition array as created by the condition() method.
...
+   * @param $condition
+   *   The condition array as created by the condition() method.
+   * @param $value
+   *   The value to match against.

Missing type hinting

+++ b/core/lib/Drupal/Core/Config/Entity/Query/Condition.phpundefined
@@ -0,0 +1,171 @@
+  protected function matchArray($condition, array $data, array $needs_matching, array $parents = array()) {
...
+  protected function match($condition, $value) {

If condition is always an array, can type hint here

+++ b/core/lib/Drupal/Core/Config/Entity/Query/Condition.phpundefined
@@ -0,0 +1,171 @@
+   * Do the actual matching.

Maybe "Performs the actual matching."? Needs a verb

+++ b/core/lib/Drupal/Core/Config/Entity/Query/Condition.phpundefined
@@ -0,0 +1,171 @@
+   * @return bool
+   *   TRUE when matches.

Blank line before @return

+++ b/core/lib/Drupal/Core/Config/Entity/Query/QueryFactory.phpundefined
@@ -0,0 +1,53 @@
+   * Instantiate a entity query for a certain entity type.

Instantiates

+++ b/core/lib/Drupal/Core/Entity/Query/QueryFactory.phpundefined
@@ -7,24 +7,24 @@
   /**
-   * var \Symfony\Component\DependencyInjection\ContainerInterface
+   * @var \Drupal\Core\Entity\EntityManager
    */
-  protected $container;

This has the @var, but no comment above it

+++ b/core/lib/Drupal/Core/Entity/Query/QueryFactory.phpundefined
@@ -7,24 +7,24 @@
+   * @param \Drupal\Core\Entity\EntityManager $entity_manager
    */
-  function __construct(ContainerInterface $container) {
-    $this->container = $container;
+  public function __construct(EntityManager $entity_manager) {

This has the @param, but no comment below it, nor a docblock for the actual method (Overrides \Symfony\...)

+++ b/core/modules/config/tests/config_test/lib/Drupal/config_test/Plugin/Core/Entity/ConfigQueryTest.phpundefined
@@ -0,0 +1,47 @@
+ *  @see \Drupal\entity\Tests\ConfigEntityQueryTest

Indented too far

+++ b/core/modules/config/tests/config_test/lib/Drupal/config_test/Plugin/Core/Entity/ConfigQueryTest.phpundefined
@@ -0,0 +1,47 @@
+   * A number used by the sort tests.
+   */
+  public $number;
...
+   * An array used by the wildcard tests.
+   */
+  public $array;

Missing @var

+++ b/core/modules/config/tests/config_test/lib/Drupal/config_test/Plugin/Core/Entity/ConfigQueryTest.phpundefined
@@ -0,0 +1,47 @@
+  public $array;
+}
diff --git a/core/modules/field_sql_storage/lib/Drupal/field_sql_storage/Entity/Query.php b/core/modules/field_sql_storage/lib/Drupal/field_sql_storage/Entity/Query.php

Missing blank line before end of class.

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/ConfigEntityQueryTest.phpundefined
@@ -0,0 +1,443 @@
+    parent::setUp();
+   $this->entities = array();

Indentation is wrong, and we usually leave a blank line after parent::setUp()

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/ConfigEntityQueryTest.phpundefined
@@ -0,0 +1,443 @@
+   * Test basic functionality.
...
+   * Test count query.
...
+   * Test dotted path matching.

Should start with "Tests"

StatusFileSize
new6.49 KB
new32.58 KB
PASSED: [[SimpleTest]]: [MySQL] 49,573 pass(es).
[ View ]

Indeed the original entity query patch didn't went through a proper doc review.

Status:Needs review» Reviewed & tested by the community

Thanks @dawehner!

This is fantastic work.

StatusFileSize
new661 bytes
new33.22 KB
PASSED: [[SimpleTest]]: [MySQL] 49,420 pass(es).
[ View ]

While we're fixing doc issues from the initial entity query patch, this missing @return has bugged me a lot in the menu links conversion :)

Issue tags:-Configuration system

Well seems a bit out of scope but I don't think this would block a release.

This looks fantastic.

Exactly the right baseline on which we may be able to advance on with faster/better/optimized implementations, but this one is guaranteed to always work.

Awesome work, everyone!

Title:Add Entity query to Config entitiesChange notice: Add Entity query to Config entities
Priority:Normal» Critical
Status:Reviewed & tested by the community» Active
Issue tags:+Needs change record

I've opened #1900962: Use Config Entity query for views_get_applicable_views(), that's a definite use case for this that can happen now.

Went ahead and committed/pushed this to 8.x to unblock other work. I'm a bit concerned about this getting abused everywhere (i..e more or less anywhere in the critical path), but we'll see.

Title:Change notice: Add Entity query to Config entitiesAdd Entity query to Config entities
Status:Active» Fixed
Issue tags:-Needs change record

The change notice is good, the random failure is patched.

The new example in the change notice does not seem to be valid PHP, maybe a wrong copy/paste?

Syntax was fixed already. It was also listing disabled views.

Priority:Critical» Normal

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