From looking at #1969698: ConfigEntity::save() should disallow saving ID/UUID conflicts (Field UUID changes can badly corrupt field data), we are injecting the entity query factory class into our controllers, so we can use entity queries to do things. However, at the moment there is no interface, so you have to type the actual factory class name. This makes it harder to switch out entity query factories, and harder to test. If someone wanted to swap out the confing QueryFactory class for instance, they would have a hard time if we type the parameters.

It makes sense to create QueryFactoryInterface that they all implement. Then it solves our problem. We can also clean up the QueryFactory classes while we're there, as config a field_storage_sql Queryfactory classes are passing the entity manager as a param to get/getAggregate, when we could just get the container to pass this into the constructor for us, as we are in the entity QueryFactory class.

Files: 
CommentFileSizeAuthor
#6 2019651-6.patch5.45 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 57,705 pass(es).
[ View ]
#6 interdiff-2019651-6.txt1010 bytesdamiankloip
#4 2019651-4.patch6.43 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 57,581 pass(es).
[ View ]
d8.QueryFactoryInterface.patch10.17 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 58,205 pass(es).
[ View ]

Comments

We don't usually add interfaces for factory classes.
Our one exception is Drupal\Component\Plugin\Factory\FactoryInterface, but that's because plugins can have swappable factories.

I don't mind this going in, I just question if we really need it.

Status:Needs review» Needs work

Yeah, there's a two-level factory in play here so interfaces are very welcome but only on the second level. The first level IMO should not be implementing this interface.

Status:Needs work» Needs review
StatusFileSize
new6.43 KB
PASSED: [[SimpleTest]]: [MySQL] 57,581 pass(es).
[ View ]

You are totally right, Let's remove any changes to the factory factory class (Drupal\Core\Entity\Query\QueryFactory). This patch just removes that, and adds the typeing back for the EntityManager that I removed for some crazy reason.

Status:Needs review» Needs work

Do we really want to inject other services without actually using them?

Status:Needs work» Needs review
StatusFileSize
new1010 bytes
new5.45 KB
PASSED: [[SimpleTest]]: [MySQL] 57,705 pass(es).
[ View ]

Whoops. Thank you!

Status:Needs review» Needs work

The last submitted patch, 2019651-6.patch, failed testing.

Status:Needs work» Needs review

#6: 2019651-6.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, 2019651-6.patch, failed testing.

Status:Needs work» Needs review

#6: 2019651-6.patch queued for re-testing.

Status:Needs review» Reviewed & tested by the community

+++ b/core/modules/field_sql_storage/lib/Drupal/field_sql_storage/Entity/QueryFactory.phpundefined
@@ -16,7 +17,14 @@
+  /**
+   * The database connection to use.
+   *
+   * @var \Drupal\Core\Database\Connection
+   */
+  protected $connection;

Even it feels a bit out of scope, that is probably fine.

Status:Reviewed & tested by the community» Fixed

Committed 0204ee8 and pushed to 8.x. Thanks!

Status:Fixed» Closed (fixed)

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

Why Drupal\Core\Entity\Query\QueryFactory is not implements this interface?

Please read the comments in this issue, this was discussed above.