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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

Priority: Normal » Major
tim.plunkett’s picture

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.

chx’s picture

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.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
6.43 KB

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.

dawehner’s picture

Status: Needs review » Needs work

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

damiankloip’s picture

Status: Needs work » Needs review
FileSize
1010 bytes
5.45 KB

Whoops. Thank you!

Status: Needs review » Needs work

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

damiankloip’s picture

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.

damiankloip’s picture

Status: Needs work » Needs review

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

dawehner’s picture

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.

tim.plunkett’s picture

alexpott’s picture

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.

andypost’s picture

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

damiankloip’s picture

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