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.
Comment | File | Size | Author |
---|---|---|---|
#6 | 2019651-6.patch | 5.45 KB | damiankloip |
#6 | interdiff-2019651-6.txt | 1010 bytes | damiankloip |
#4 | 2019651-4.patch | 6.43 KB | damiankloip |
d8.QueryFactoryInterface.patch | 10.17 KB | damiankloip | |
Comments
Comment #1
xjmBlocks a major => major. (From #1969698: ConfigEntity::save() should disallow saving ID/UUID conflicts (Field UUID changes can badly corrupt field data).) Thanks @damiankloip!
Comment #2
tim.plunkettWe 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.
Comment #3
chx CreditAttribution: chx commentedYeah, 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.
Comment #4
damiankloip CreditAttribution: damiankloip commentedYou 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.
Comment #5
dawehnerDo we really want to inject other services without actually using them?
Comment #6
damiankloip CreditAttribution: damiankloip commentedWhoops. Thank you!
Comment #8
damiankloip CreditAttribution: damiankloip commented#6: 2019651-6.patch queued for re-testing.
Comment #10
damiankloip CreditAttribution: damiankloip commented#6: 2019651-6.patch queued for re-testing.
Comment #11
dawehnerEven it feels a bit out of scope, that is probably fine.
Comment #12
tim.plunkettSee #2021111: Add a ConfigFactoryInterface for ConfigFactory for another
Comment #13
alexpottCommitted 0204ee8 and pushed to 8.x. Thanks!
Comment #15
andypostWhy
Drupal\Core\Entity\Query\QueryFactory
is not implements this interface?Comment #16
damiankloip CreditAttribution: damiankloip commentedPlease read the comments in this issue, this was discussed above.