derivative paths should be taken from the corresponding original file, and not from the default

As much as I appreciate your effort here, I have to mark this issue as duplicate of #103793: Add token support for image file directory and image names. This is a long-wanted feature and I can't await to see a proper patch for it.

Reopening and renaming.
As discussed on http://drupal.org/node/103793, this is a bug:

"derivative paths should be taken from the corresponding original file, and not from the default. This should ensure, that derivatives are always sticking to the original's location, whatever I do with the setting later."

JirkaRybka, could you reroll this patch so it just keeps derivatives in the same folder as originals? ie don't put the derivatives into subdirs.

Sorry for the noise.

I agree with the separation into this issue. However, I am not entirely sure whether this is really how it should work. I wonder whether this works with all edge-cases. Because _image_filename() is invoked from a lot of places.

Also: Does this mean that users cannot alter the path for existing images?

Maybe I just need to be convinced.

In any case, this needs SimpleTest(s).

I haven't looked at the code in depth, but from what the original report says, existing images aren't moved when you change the value of the image path. (I think I saw a bug report or a forum post on that.) The image path settings doesn't say 'this is where all images ARE', it says 'this is where to PUT new images'.
Regenerated derivatives are not really new images, and it makes sense to keep them in the same folder as the original.
We could decide in future that changing the image path setting MEANS moving all images -- but that's a new feature and would be quite a bit of work. (And think of how long it could take on big sites!)

As well as tests, this could do with a preliminary patch to add more documentation to _image_filename(), listing where it is invoked from.

Whoa! This comment almost made me push the button without reading it completely :-D Thanks! You rock.

So the remaining todos are:

- Tests to ensure this works as intended.

- Rename the function argument to $filepath.

- PHPDoc teaching that $filepath should be a full path with filename.

- Revert the placement of the "Insert the resized name in non-original images" comment.

- Rework the inline comment to exactly state the sanity checks you noted here for future reference. Remove the extra "temp" and "tmp" checks. The new tests for this should verify that the logic works properly.

- Stop bashing Image Assist ;)

@JirkaRybka: do you still have time to work on this? If not, I can take a look at it :)
@sun: could you clarify what a test should do? My first guess would be: set the filepath to 'images1', create an image, change the filepath to 'images2', regenerate the derivatives for the image and check they remain in 'images1'. Is this what you had in mind?

I'd like to get this fixed so we can move forward with token support for original image filepaths here: http://drupal.org/node/103793

I'd like to help test this, but I'm new to this whole patch process. I figured out how to do the patching, went with Cygwin, and did successfully patch it, at least it said so, but then what? Do I run update.php after patching, or is it good to go?

2noame: it's good to go. Try things out and report any new problems here.

Ok I've got part of the way with a test but I am stumped.

  /**
   * Verify derivatives take their path from the original image.
   */
  function testImageDerivativePath() {
    // Create an image.
    $edit = array(
      'title' => $this->randomName(),
      'body' => $this->randomName(),
      'files[image]' => realpath($this->image),
    );
    $this->drupalPost('node/add/image', $edit, t('Save'));
    
    $node = node_load(array('title' => $edit['title'])); 
    file_put_contents('hello_1', print_r($node,1));
    
    // Path of the thumbnail derivative.   
    $first_thumbnail = $node->images['thumbnail'];
    
    // Change the file path.
    variable_set('image_default_path', 'images_alternate');
    
    // Rebuild derivatives.
    image_operations_rebuild(array($node->nid));
    
    $node = node_load(array('title' => $edit['title']));     
    file_put_contents('hello_2', print_r($node,1));
    // ARG! Has NO IMAGES!
    
    $new_thumbnail = $node->images['thumbnail']; 
    
    $this->assertTrue($first_thumbnail == $new_thumbnail, t("$first_thumbnail == $new_thumbnail"));  
    
  } 

Just paste the above as a new function into the class in tests/image.test.
Basically, image_operations_rebuild is killing the images but not rebuilding! It seems to work when invoked from admin/content/node, but it seems when called directly the node is not getting saved.
Not sure how it's mean to work as image_update doesn't do any kind of node saving from what I can see.

Could really use a fresh pair of eyes on this!
Just try the above text code, no need to try the patch yet. The test should fail due to the two thumbnail files being different rather than the second one being absent!

Was this included as a fix in the newest alpha 5? If not, it really needed to be.

If this issue doesn't say 'fixed' then no fix was included -- we work entirely within the issue queue.
See above comments for how far work got on this. Help is needed.

JirkaRybka, your patch works beautifully and thank you so much for it. You're a lifesaver. I too have tens of thousands of images spread across dozens of subfolders and as it stood without this bugfix, and yes I consider it a bug to be fixed, image module would have remained unusable to me and I would have been forced to dump image module in favor of imagecache/imagefield.

This really does need to be added to the next image module update because thumbnails do belong alongside their original files. Without it, anyone using multiple directories is going to get a crazy large derivatives directory and anytime that folder gets changed to a new one, all the images require being remade yet again. This patch fixes that. The only thing I needed to do other than install this patch, was to delete all former derivatives and let them all get remade properly.

Also, just a note, I use this in combo with Lightbox2 and prior to patching, the ability to do full size downloads was broken whenever the derivatives were not in the same folder as the original. So this patch corrected that too. At this point everything is working exactly the way I want it to.

Thanks again!

Having a long-term interest in #103793: Add token support for image file directory and image names and Image in general I took a look at the last patch and have tried to apply the changes from #6 as I understand them.

I have also put together what I think is a pretty solid test function, based on the suggestion in #12 but using form posts to change the image file path and to rebuild a node's derivatives rather than setting variables or calling functions directly.

I have also had to make a slight change to the last test function in the script as it was picking up the values of the second node created by my new test, rather than the second node created by it's own actions. This was due, I think, to cacheing. I put my new test in what I thought was a suitable position based on the general flow of the testing steps.

I also had to add some access permissions, one to support my test and the others because certain tests have I think been failing since #44057: Use core-style content permissions was committed.

I hope my offering is useful. Unfortunately I will not be around to respond to comments etc. for the next week or so.

On reflection I think a status change is appropriate.

Wahey!

I've split this patch up:
- First: the fix to permissions: that's a fix to the changes in #44057: Use core-style content permissions, so I've committed it there.
- Simpletest: the changes to the test suite.
- Image: the change to the module code.

So:
- applied the simpletest patch and the new test FAILS. This is expected and good -- the new test should fail with the current code.
- applied the image patch on top of this, and the new test PASSES. A manual test drive of this confirms it works -- regenerating a node after changing the image path leaves the derivatives in the original location.

So a +1 from me, but I'd like at least one other person to test it.
Though I'd also like to get a RC released on the first day of DrupalCon at the latest, so... :)

+++ tests/image.test	14 Aug 2009 08:18:45 -0000
@@ -196,6 +197,81 @@ class ImageTestCase extends DrupalWebTes
+   
...
+   

Trailing white-space (blank lines should be blank).

+++ tests/image.test	14 Aug 2009 08:18:45 -0000
@@ -196,6 +197,81 @@ class ImageTestCase extends DrupalWebTes
+    $this->assertRaw(t('@type %title has been created.', array('@type' => 'Image', '%title' => $edit['title'])),
+      t('Image node was created.'));
...
+    $this->assertRaw(t('@type %title has been created.', array('@type' => 'Image', '%title' => $second_edit['title'])),
+      t('Second image node was created.'));
...
+    $this->assertTrue(preg_match('/images_alternate/', $second_node->images['_original']),
+      t("New path {$second_node->images['_original']} used for second image."));
...
+    $this->assertRaw(t('@type %title has been updated.', array('@type' => 'Image', '%title' => $edit['title'])),
+      t('First image node was updated to rebuild derivatives.'));
...
+    $this->assertTrue($node->images['thumbnail'] == $first_node->images['thumbnail'],
+      t("Rebuilt derivatives for first image have same thumbnail path as original node: {$node->images['thumbnail']} == regenerated thumbnail path {$first_node->images['thumbnail']}")); 

Strange wrapping here. Let's just keep it on one line. ;)

This review is powered by Dreditor.

+++ tests/image.test	14 Aug 2009 08:18:45 -0000
@@ -196,6 +197,81 @@ class ImageTestCase extends DrupalWebTes
+   
...
+   

Trailing white-space (blank lines should be blank).

+++ tests/image.test	14 Aug 2009 08:18:45 -0000
@@ -196,6 +197,81 @@ class ImageTestCase extends DrupalWebTes
+    $this->assertRaw(t('@type %title has been created.', array('@type' => 'Image', '%title' => $edit['title'])),
+      t('Image node was created.'));
...
+    $this->assertRaw(t('@type %title has been created.', array('@type' => 'Image', '%title' => $second_edit['title'])),
+      t('Second image node was created.'));
...
+    $this->assertTrue(preg_match('/images_alternate/', $second_node->images['_original']),
+      t("New path {$second_node->images['_original']} used for second image."));
...
+    $this->assertRaw(t('@type %title has been updated.', array('@type' => 'Image', '%title' => $edit['title'])),
+      t('First image node was updated to rebuild derivatives.'));
...
+    $this->assertTrue($node->images['thumbnail'] == $first_node->images['thumbnail'],
+      t("Rebuilt derivatives for first image have same thumbnail path as original node: {$node->images['thumbnail']} == regenerated thumbnail path {$first_node->images['thumbnail']}")); 

Strange wrapping here. Let's just keep it on one line. ;)

+++ image.module	13 Aug 2009 20:45:52 -0000
@@ -861,17 +861,29 @@ function _image_build_derivatives($node,
+ * @param $filepath
+ *   The full path and filename of the original image file.
...
-function _image_filename($filename, $label = IMAGE_ORIGINAL, $temp = FALSE) {
+function _image_filename($filepath, $label = IMAGE_ORIGINAL, $temp = FALSE) {

The remaining arguments are not covered.

I didn't look this up, but is it ensured that all functions really pass a filepath here (instead of a filename)?

This review is powered by Dreditor.

One other point now I see your review:

+ * The full path and filename of the original image file.

I always get very confused with paths -- are they relative or absolute?
I've found working on module builder that you can use a path relative to Drupal root or an absolute one in most cases for PHP functions.
But it's worth saying in the function comment header, just to be clear. Eg:

+ * The full path and filename of the original image file, relative to Drupal root, eg 'sites/default/files/images/myimage.jpg

> The remaining arguments are not covered.

It was like that before though -- patch creep kills kittehs! ;)

> I didn't look this up, but is it ensured that all functions really pass a filepath here (instead of a filename)?

$ grep -r _image_filename *
image.module:    $destination = _image_filename($original_path, $key, $temp);
image.module:function _image_filename($filepath, $label = IMAGE_ORIGINAL, $temp = FALSE) {
image.module:  if (file_move($image_path, _image_filename($original_path, $size))) {
image.module:  if (!file_copy($filepath, _image_filename($filepath, IMAGE_ORIGINAL, TRUE))) {

Looks ok :)

Since the tweaks are all stylistic, I'll make them once we get another test review.

This patch is essential to a 1.0 release of image module.
I intend to commit it while at DrupalCon Paris, regardless of its status, prior to making a 1.0 release
If you care about the quality of image module, please try to test it before then.

I tested the new patches in #19 as suggested:

• Install new D6.13, Image HEAD etc.
• Apply testing patch
• Run Simpletest for Image - test fails as expected
• Apply image patch
• Run Simpletest for Image again - test succeeds as expected.

I also did a manual test, changing the Image path after uploading an image and rebuilding the image node derivatives. The derivatives were rebuilt as expected.

It all checks out as far as I can see.

> Strange wrapping here. Let's just keep it on one line. ;)

FWIW, why is it ok to have lines of code that wrap horrendously, but not lines of doxygen comments?
The former are hard to read, the latter a pig to write and update.
I've changed it as requested but I for one rather like to cut up long function calls into one parameter per line.

Anyway.... :)
Changes as detailed above made, committed patches.

Thank you for coding and testing sp3boy! :D

Thanks! :)

To my knowledge, there are two ways to write lengthy lines without looking strange:

1) Single line.

$this->assertTrue($node->images['thumbnail'] == $first_node->images['thumbnail'], t("Rebuilt derivatives for first image have same thumbnail path as original node: {$node->images['thumbnail']} == regenerated thumbnail path {$first_node->images['thumbnail']}"));

2) Multi-line.

$this->assertTrue(
  $node->images['thumbnail'] == $first_node->images['thumbnail'],
  t("Rebuilt derivatives for first image have same thumbnail path as original node: {$node->images['thumbnail']} == regenerated thumbnail path {$first_node->images['thumbnail']}")
);

2) is most often used for arrays. But Drupal HEAD uses it also in some other places for function arguments, so it seems to have become a valid coding standard for better readability, for example:

http://api.drupal.org/api/function/node_delete_confirm/7

Apparently, it looks like all invocations of confirm_form() have been converted to multi-line syntax.

The difference to partially wrapping arguments/array-elements is that the code follows a consistent standard. That means, a script like coder_format (which we still need to apply on all image module files) is able to properly figure out, which standard has been applied, to reformat the code accordingly. The same machine-made assumptions done by coder_format are also done by all humans.

Well done getting this one fixed everyone! Now to get token support.... :D

For reference, that's here: #103793: Add token support for image file directory and image names.

@JirkaRybka ...
Off-topic, perhaps it deserves its own discussion elsewhere, but...

The more we split lines, the less code fits onto one screen, eventually hiding context

If we don't split lines, actual code is actually hidden by being invisible off-screen.
Done with sane indenting, IMO it preserves context a lot better than minifying it does.

I split my lines overmuch, because I like to be able to read them at a glance, and I have a scrollwheel on my mouse. And an IDE that makes jumping through files a non-issue. Were I using vi, I would reconsider.

I most often do it inside a t() function (twice for the nested array) so I can see the values on-screen.
It may not be concise, but it makes it a lot more approachable when looking at someone elses code and groking it, knowing that you haven't missed something off-screen, or had to weed out and count the commas in one big string.

Which of the following is more obvious in intent, and easier to spot a problem in?

      return '<p>'. t("[meta_descriptionfile.module] If there is a %descriptionfile available in the import directory, that may be used to easily provide captions for the found images. <a href='!help'>About %descriptionfile</a>", array('%descriptionfile' => DESCRIPTIONFILE_METADATA_FILENAME, '!help' => url('admin/help/meta_descriptionfile'))) .'</p>';

      return "<p>". t("[meta_descriptionfile.module] 
        If there is a %descriptionfile available in the import directory, 
        that may be used to easily provide captions for the found images. 
        <a href='!help'>About %descriptionfile</a>", 
        array(
          '%descriptionfile' => DESCRIPTIONFILE_METADATA_FILENAME, 
          '!help' => url('admin/help/meta_descriptionfile')
        )) ."</p>";

... I'm not going to advocate that splitting my strings just for readability is a great idea ... but it helps me keep my embedded documentation up to date.