Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem
- Image module implements its own image style hooks in order to perform essential massaging of ImageStyle entity properties.
Solution
- Add a new
ImageStyleStorageController extends ConfigStorageController
to perform those operations.
Comment | File | Size | Author |
---|---|---|---|
#33 | 1821848-image-33.patch | 10.08 KB | alexpott |
#24 | 22-24-interdiff.txt | 3.79 KB | alexpott |
#24 | 1821848-image-24.patch | 9.78 KB | alexpott |
#22 | 1821848-image-22.patch | 7.86 KB | andypost |
#19 | 1821848-image-18.patch | 1.01 KB | andypost |
Comments
Comment #1
japicoder CreditAttribution: japicoder commentedI start to work on it
Comment #2
japicoder CreditAttribution: japicoder commentedFinally I'm not able to start this task. I can't figure how the ConfigStorageController is going to work and after some frustrating hours, I release to anonymous so maybe later I can do more with it or another person with more knowledge than me.
Comment #3
andypost@japicoder you could take example from
#1497380: Convert shortcut sets to ConfigEntity #1552396: Convert vocabularies into configuration
or
/core/modules/views/lib/Drupal/views/ViewStorageController.php
Comment #4
rbayliss CreditAttribution: rbayliss commentedHere's a start.
Comment #5
tim.plunkettIt seems this file is missing? Try
git add .; git diff --staged
Comment #6
rbayliss CreditAttribution: rbayliss commentedOh, right. Sorry about that.
Comment #8
rbayliss CreditAttribution: rbayliss commented#6: image_storage_controller-1821848-6.patch queued for re-testing.
Comment #9
gddI remember having this conversation with timplunkett when I was doing the conversion, and we both agreed it really didn't matter whether we implement hook_ENTITY_TYPE() or extend the storage controller. I seem to remember that at the time Views was doing the former, although looking now they aren't. I also had this conversation with fubhy around #1479454: Convert user roles to configurables. I personally prefer the hook implementations because the code is in the same file as the main implementation, which makes it a lot easier to find. Is there a significant advantage either way to moving this code to the storage controller?
Comment #10
sunHooks are easier to find...? If you look at this patch, then you see a storage controller for image styles, which contains the entire CRUD logic for image styles. Discovery of that is barely beatable. ;)
That's only the secondary advantage though. The primary advantage is that image styles are (or should not) actually be special and require tons of custom storage code — as long as image styles need that, we can be sure that almost all other config entities will need that, too.
Therefore, the other goal here is to get a better overview of what logic is in the ImageStyleStorageController, and ultimately, which parts of that could be simplified/generalized for all config entities.
One part there is definitely going to be the rename/replace logic (which wasn't moved to the storage controller in this patch yet for some reason, but it should), which is a concept that is currently known to be special for image styles, but actually, it is quite foreseeable that many other config entities will have a need for that, too.
Comment #11
gddThey are easier to find if you're familiar with the architecture of entity storage. If you're not then they are incredibly difficult to find.
Anyways, I appear to be in the minority on this one, and I honestly don't have a horribly strong feeling about it. The patch is straightforward. Just gonna give the bot another run at it and if its green I'm fine.
Comment #12
gdd#6: image_storage_controller-1821848-6.patch queued for re-testing.
Comment #13
sun@rbayliss: Was there any particular reason for not including the replace code in the controller?
I think we want to put that into a dedicated replace() method even (only on this particular controller for now), and call that method from the delete form submission handler, which gets the replacement ID.
Lastly, there are various coding style issues with the current patch, which we need to clean up.
Comment #14
rbayliss CreditAttribution: rbayliss commentedNot that I can think of. I remember being confused about how the replace code fits in with the config import/export API. Other than that I guess I just didn't think to do it.
Comment #15
tim.plunkettThere is now an ImageStyleStorageController, so this needs a reroll.
Though it also might need to be redone after #1782244: Convert image styles $style array into ImageStyle (extends ConfigEntity) as well.
Comment #16
andypostit seems everything was fixed already, so we can close this one!
Comment #17
andypostThe only hunk that lost... Suppose this issue major because calling not-existent function
Comment #18
andypostFixed comment.
Remaining hooks uses functional style of operation
image_image_style_load() -
image_effect_definition_load($effect['name'])
image_image_style_update() - depends on field module (could be fixed here)
image_image_style_delete() - uses image_image_style_update()
Comment #19
andypostPatch itself
Comment #20
sunI don't see the remaining parts of the patch in #6 in HEAD, so I think the entire original patch still needs to be done.
E.g.,
image_image_style_load()
still exists, but effects should be prepared by the storage controller already, before the image style is passed to hook implementations.Same applies to the other hooks that Image module implements for itself.
Comment #21
andypost@sun agree but #19 is required, probably we have no tests for this
Comment #22
andypostSo moved all the logic to storage controller
Also fixed flushing media for
importDelete()
.Comment #23
andypost#22: 1821848-image-22.patch queued for re-testing.
Comment #24
alexpottNow that we have a postDelete() method the whole importDelete method is superflous. I've added a test to ensure that a configImport() does the right thing for image style deletes.
Also this issue is a bug because the call to the non-existant function
image_style_delete()
that's currently inImageStyleStorageController::importDelete()
would cause a fatal error.Also the @todo in the following code...
is completely spurious because when you do a delete that has a replacement style you will delete the config file... therefore you can not know the replacement style when you import to another site because the config no longer exists.
As this was a re-roll the interdiff is not a true interdiff but based on my git history...
Comment #25
andypostLet's get this in
Comment #26
xjm#24: 1821848-image-24.patch queued for re-testing.
Comment #28
alexpottThe fail was in our old friend
Drupal\translation_entity\Tests\EntityTestTranslationUITest
... re-testingComment #29
alexpott#24: 1821848-image-24.patch queued for re-testing.
Comment #30
alexpottSetting back to RTBC as the fail was due to a random unrelated fail... :(
Comment #31
catch#24: 1821848-image-24.patch queued for re-testing.
Comment #33
alexpottRerolled due to configentity rename change
Comment #34
xjmLooks great to me.
Comment #35
catchCommitted/pushed to 8.x, thanks!
Comment #36
andypostSeems we need a follow-up to get rid of this
Comment #37
alexpott@andypost how come?