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/Motivation
Right now focal_point_install() and focal_point_update_7001() create a test drive image in the public files directory. This breaks on sites whose file default scheme is remote (such as amazons3.module's s3://).
Proposed resolution
Move focal_point_update_7001()'s code to a new parameterized helper function, called from both focal_point_update_7001() (so old functionality remains the same), and also add a new update hook calling the same function with the file_default_scheme variable.
Remaining tasks
Patch- Review
Test
User interface changes
The test image should now appear on sites with remote stream wrappers and multiple webs.
API changes
None.
| Comment | File | Size | Author |
|---|---|---|---|
| #9 | update_test_drive_image-2453429-9.patch | 1.93 KB | bleen |
| #2 | interdiff-2453429-1-2.do-not-test.diff | 484 bytes | scottrigby |
| #2 | focal_point-file_default_scheme-2453429-2.patch | 2.13 KB | scottrigby |
Comments
Comment #1
scottrigbyPatch attached. Tested the update locally, on a site configured with amazons3.module and
file_default_schemeset tos3. Did not go back and re-testfocal_point_update_7001()but the actual code has not changed (since 'public is passed').Comment #2
scottrigbyWhoops I didn't change to the new update in
hook_install(). Updated patch and interdiff attached.Comment #3
bleen CreditAttribution: bleen commentedis this change needed, given 7004?
Comment #4
scottrigby@bleen nah, I was just following the protocol of never changing update hook history (I did change the code itself, putting DRY over that rule of thumb… the result is the same). I agree though it's kinda useless given 7004.
Also note new org attribution :)
Comment #5
scottrigby@bleen18 if you want me to kill it, we could just keep 7001 with a code comment explaining the reasoning for the change? Or we could leave it. Up to you
Comment #6
bleen CreditAttribution: bleen commented@scottrigby ... yeah, I feel dirty changing an old update (except for comments)
Comment #7
tklawsuc CreditAttribution: tklawsuc commentedI want to add that this is also preventing an install profile from loading up. When focal_point is added as a dependency in the profile, the installation fails (using drush si):
The bundle that it's looking for is "type".
When I comment out the logic in focal_point_update_7001(), the installation works fine. Can you consider maybe adding the test image when the test page is accessed for the first time instead of on install?
FYI using v 7.x-1.0-beta4 and drupal 7.37.
Comment #8
bleen CreditAttribution: bleen commented@tklawsuc... I think you are describing a different issue. Are you saying that the patch in #2 solves your issue or just that killing 7001 (without introducing 7004) solves your issue. If its the former, would you please open a separate issue? If it's the later I'll be really surprised.
Comment #9
bleen CreditAttribution: bleen commented@scottrigby ... I've reconsidered my comment in #3 after looking at how updates like this are handled in the system module (at least in D7). This patch, which i committed, simply removes update_7001 completely.
The substance of this patch is a good fix. Thanks!!
Comment #11
tklawsuc CreditAttribution: tklawsuc commentedI tried the patch and it still fails so there is a different issue. I will create a separate ticket...in the meantime I just removed the call to focal_point_create_test_image on install. Thanks.
Comment #13
Daniel KorteI can confirm this patch fails if the module is already installed and uses the public:// scheme. Here's the error:
Failed: PDOException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry 'public://focal_point/test-drive.jpg' for key 'uri': INSERT INTO {file_managed} (uid, filename, uri, filemime, filesize, status, timestamp, type) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5, :db_insert_placeholder_6, :db_insert_placeholder_7); Array ( [:db_insert_placeholder_0] => 1 [:db_insert_placeholder_1] => test-drive.jpg [:db_insert_placeholder_2] => public://focal_point/test-drive.jpg [:db_insert_placeholder_3] => image/jpeg [:db_insert_placeholder_4] => 497845 [:db_insert_placeholder_5] => 1 [:db_insert_placeholder_6] => 1442523358 [:db_insert_placeholder_7] => image ) in drupal_write_record()
So basically the update is trying to create a duplicate of the test image with the same URI. The update function should probably delete the previous test image or check that the same one doesn't already exist.
Comment #14
muschpusch CreditAttribution: muschpusch commented+ 1 having the same issue as Daniel Korte. The release of today brings this patch in. There doesn't seem to be an issue created by tklawsuc. Should we re-open this one or create a follow up?
Comment #15
bleen CreditAttribution: bleen commented@muschpusch .. please create a followup (thats why I missed it before todays release :( )
Comment #16
scottrigby@Daniel Korte & @muschpusch That makes sense. Can you relate the new issue to this one once created so people can follow the trail? Thanks
Comment #17
muschpusch CreditAttribution: muschpusch commented#2655598: Update hook focal_point_update_7004 failing