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.
The attached patch should hopefully resolve a couple of problems I had getting Filefield_paths to tokenise image filenames and paths:
- by calling
node_save()
inimage_operations_rebuild()
instead ofimage_update()
, the node api hooks are run, allowing Filefield_paths to override the image's file / path name. - by not clearing $node->rebuild_images in
image_update()
, the object property is still set when Filefield_paths goes to work on it (see related patch in #564680: Add support for bulk rebuilds of derivative files in Image module).
Comment | File | Size | Author |
---|---|---|---|
#25 | image-564690-25.patch | 6.17 KB | sp3boy |
#23 | image-564690-15-unix-format.patch | 6.17 KB | sp3boy |
#20 | img_assist.module.6.x-1.1.patch | 2.88 KB | ibis |
#15 | image-564690-15.patch | 6.33 KB | sp3boy |
#14 | image-564690-14.patch | 5.82 KB | sp3boy |
Comments
Comment #1
joachim CreditAttribution: joachim commented> by calling node_save() in image_operations_rebuild() instead of image_update()
Thing is, we call image_update in TONS of places to force a rebuild of derivatives.
It's pretty ugly -- precisely because of this sort of thing. Hook implementations should only be hook implementations.
There's a case for refactoring and having a nice _image_rebuild_derivs() function that's safe to call whenever -- but that's a big job and it's spaghetti in there, and we're near to a release.
Comment #2
sp3boy CreditAttribution: sp3boy commentedI'm not sure whether I should be looking at other modules as well (suggestions?) but in the current Image HEAD, image_update() is only called twice: once in the
image_operations_rebuild()
and also inimage_load()
(the call to which will move to another function if #226121: don't manipulate images on hook_load is adopted).The setting of
$node->rebuild_images
does happen in quite a few places within image.module, sure.Doing the search for
image_update()
has got me thinking that if it were to be replaced bynode_save()
it should be done consistently, hence the replacement should also be inimage_load()
(as-is). I will look into this.Comment #3
sp3boy CreditAttribution: sp3boy commentedVolunteering status change :)
Comment #4
joachim CreditAttribution: joachim commentedCheck all the submodules -- attach, gallery, etc.
Comment #5
joachim CreditAttribution: joachim commentedCheck all the submodules -- attach, gallery, etc.
Comment #6
sp3boy CreditAttribution: sp3boy commentedFWIW I did search all the contrib submodules and for example,
image_create_node_from()
in image_attach.module calls node_save().BTW is anyone else experiencing serious timeout problems with D.O in recent days? I couldn't help notice the double posting above...
Comment #7
joachim CreditAttribution: joachim commentedYup, pretty much everything broke on teh internets yesterday for people at Drupalcon.
Comment #8
sp3boy CreditAttribution: sp3boy commentedJust to confirm that I am still on the case and it has become apparent that calling
node_save()
instead ofimage_update()
in the recently-introduced_image_build_derivatives_if_needed()
is not a good idea. I had not forseen the implications of callingnode_save()
within a hook, itself run withinnode_load()
. In practice I think filefield_paths attempts to substitute token values which are derived from node properties at a point when they have not yet have been populated on the node object.So it may be workable to call
node_save()
inimage_operations_rebuild()
for the benefit of filefield_paths but to continue callingimage_update()
when missing derivatives need to be reconstituted (which as we know from #411568: derivative paths should be taken from the corresponding original file, and not from the default should be happening in the same path as the "original" image file).I will continue investigating and BTW it's great to see beta1 released!
Comment #9
sp3boy CreditAttribution: sp3boy commentedHere is a revised patch. The main change is that in order for Filefield_paths to work effectively with Image, I believe it is essential that for Image files, the {files}.filename column contains the name of the file, not the derivative size label as Image currently stores. Hence there is:
_image_insert()
to store the filename not the size in {files}.filenameimage_update_6104()
function which updates any existing image files in the database.(This main change effectively takes over from the similar discussion in #424386: images saved to files table with bad data in filename column I think.)
There is still a change to
image_operations_rebuild()
so that bulk updates are processed withnode_save()
notimage_update()
, as mentioned previously.In a couple of places I have had to add some code that is conditional on Filefield_paths being installed, so that the {files}.origname is retrieved to ensure that derivatives are created with the file's original name, to allow Filefield_paths to use that when tokenising the derivatives.
In
image_create_node_from()
I have added a call to_image_build_derivatives()
to ensure that the derivative files physically exist before thenode_save()
is performed.I have tested all the above (in conjunction with #564680: Add support for bulk rebuilds of derivative files in Image module) exhaustively, but of course it would be silly to claim it is water-tight and I plan to test some more. However I do think that this patch - or the features in it - are essential to complete (hopefully) the integration of Image and Filefield_paths.
Comment #10
scroogie CreditAttribution: scroogie commentedThis seems to make more sense than storing the size in the filename column. Hopefully I'll have time to test it. Any corner cases I should try out?
Comment #11
sp3boy CreditAttribution: sp3boy commentedI've tested with basic image upload, image attach and image import. I haven't tested with imagecache and/or other modules that depend on Image so if anyone is able to it might be worth it.
Having said that, I am already by necessity running this patch together with #564680: Add support for bulk rebuilds of derivative files in Image module on two live sites (due to lack of any other option functionality-wise).
Comment #12
joachim CreditAttribution: joachim commented> In image_create_node_from() I have added a call to _image_build_derivatives() to ensure that the derivative files physically exist before the node_save() is performed.
I'm increasingly confused about all the different places _image_build_derivatives() is called from and the flow around node creation. Doesn't this mean that a flow coming in from image_create_node_from() will get derivatives built twice?
Comment #13
sp3boy CreditAttribution: sp3boy commentedAs I understand it, in order of appearance in image.module:
_image_build_derivatives()
is called duringimage_update()
which is itself called fromnode_save()
. My patch introducednode_save()
inimage_operations_rebuild()
instead ofimage_update()
to try and run the same functions for a bulk operation as for a node-edit-and-save sequence._image_build_derivatives()
is called duringimage_validate()
_image_build_derivatives()
is called via_image_build_derivatives_if_needed()
duringimage_load()
The problem I was having is that Image Import and Image Attach use
image_create_node_from()
but rely on the derivatives being created on the fly when the node is loaded again.I thought the solution was to invoke
_image_build_derivatives()
inimage_crate_node_from()
so that reliance is eliminated.However in the course of refreshing my memory to give you an answer, I think I have spotted a little problem.
Yes, we can ensure that when Image Import and Image Attach use
image_create_node_from()
it creates derivatives before thenode_save()
, however I think there is still an issue with re-creating missing derivatives. This is done with the build-on-the-fly approach too, which usesimage_update()
but does not perform anode_save()
, hence may not invoke Filefield_paths.I will re-test that side of things tomorrow and report back as soon as I can.
Comment #14
sp3boy CreditAttribution: sp3boy commentedThere was a problem in that missing derivatives were created during node load with their original filename if Filefield_paths was installed and configured to rename image files. I'm not sure how I missed that in all the testing.
The approach of the attached patch in respect of this is now to:
_image_build_derivatives()
inimage_create_node_from()
. This should avoid performance issues in the event of a large image import._image_build_derivatives_if_needed()
so that, if it decides that some derivatives are missing (which is the case during the first node load directly after an image attach or image import) set a property on the node object to indicate that anode_load()
is in progress. This property is tested by_image_build_derivatives()
and_image_insert()
and in each case, if it is set, the function does not attempt to look up the original name of the image file but creates derivatives based on the current name of the "original" image. Hence the derivatives match the original without having to invoke the Filefield_paths hooks.Comment #15
sp3boy CreditAttribution: sp3boy commentedI found another little problem with the previous patch for the following case:
_image_build_derivatives_if_needed()
to cause their re-creationThe attached patch takes a more sophisticated approach in
_image_insert()
:_image_filename()
and that is assigned to the origname column when populating {files} (addition to previous patch)Apologies to anyone who has spent time testing the previous patches (?anyone...), the goal is still a bug-free integration of Filefield_paths and Image.
Comment #16
sp3boy CreditAttribution: sp3boy commented#564680: Add support for bulk rebuilds of derivative files in Image module has been commited to Filefield_paths, as has #373094: Retroactive like behavior when changing token source which allows path changes to be applied automatically when a node is edited.
I have done more testing and have no outstanding problems with the last patch submitted for this issue.
Comment #17
ibis CreditAttribution: ibis commentedI tested the latest patch on 6.x-1.0-beta5 with Filefield_paths 6.x-1.x-dev (2010-01-11) and Image Fupload 6.x-3.x-dev (2009-09-29) and work fine!
But. Many module that's interactive with Image module (eg. Lightbox2) uses files.filename as information of file's size and produced malfunction. To my mind this is very important patch and i suggest to commit in the next release, but with one announcement on module's page to must review the modules that's interact's with Image module.
Comment #18
sp3boy CreditAttribution: sp3boy commentedThank you for taking an interest. I use Lightbox2 on all my sites and have not noticed a problem so far. I will look into it some more though.
Comment #19
sp3boy CreditAttribution: sp3boy commentedAs far as I can see with Lightbox2, the only reference to the 'filename' field is for checking the file extension of files related to the Filefield module, not the Image module (lightbox2.module functions
lightbox2_check_filefield_extension
andlightbox2_check_filefield_image_extension
). If anyone can suggest actual circumstances where the change in use of {files}.filename causes a problem in other modules, please give some details.Comment #20
ibis CreditAttribution: ibis commentedPatch to img_assist module ver 6.x-1.1, if you applied the patch in #15
Comment #22
joachim CreditAttribution: joachim commentedI think one of the reasons this issue is dragging on is that the whole flow around derivative creation/re-creation is tangled and scary. This is certainly what puts me off reviewing this.
I wonder whether we should file a separate issue to look at that, figure it out, and either document it better or untangle it.
Comment #23
sp3boy CreditAttribution: sp3boy commentedWell here's a version of the #15 patch that has the required line terminators, which seems to be an issue with testing.
I agree that when combining Filefield_paths with Image, the derivative scenario gets more complicated. Even the terminology for derivatives is confusing what with ORIGINAL and origname relating to different things.
However I have been using my last patch on live sites since shortly after the time it was posted... because there really wasn't an alternative what with #103793: Add token support for image file directory and image names being dead in the water.
Comment #24
joachim CreditAttribution: joachim commentedWouldn't this be better as !isset() ?
What's f.origname?
Does Filefield_paths add a column to {files}? Please tell me it doesn't!!!
It would be nice to have a longer name without abbreviations, but if this is the DB field name, should probably leave it to match that.
A comment to explain what this variable will hold would be good though.
If you are retrieving only one row, and only one field, then use db_result() (IIRC... or some similar name). Saves you a variable.
This review is powered by Dreditor.
Comment #25
sp3boy CreditAttribution: sp3boy commentedThanks for the input. Taking your most significant point, yes I'm afraid Filefield_paths does add a column to {files}. Not as a result of my efforts but - I assume - as a long-term requirement.
I have adopted your suggestions and attach a new patch.
Comment #26
mattab CreditAttribution: mattab commentedI'm using D7, a Content type with an Image field and a Taxonomy term. I want to rename my photo to be [node:field_country:term]
Currently Filefield path doesn't seem to work as my photo keeps its name "2.jpg"
What does this patch do exactly and maybe it would help my issue? thanks!
Comment #27
joachim CreditAttribution: joachim commentedImagefield on D7 is nothing to do with this module... :)
Comment #28
fishclic CreditAttribution: fishclic commentedPatch on comment #25 works perfectly, thanks ! :)