CRM-20821 & CRM-20938 - fix bugs in managing premium product images #10720
Conversation
|
Jenkins re test this please |
| $params['is_deductible'] = CRM_Utils_Array::value('is_deductible', $params, FALSE); | ||
| // Use local URLs for images when possible | ||
| $params['image'] = CRM_Utils_String::simplifyURL($params['image'], TRUE); | ||
| $params['thumbnail'] = CRM_Utils_String::simplifyURL($params['thumbnail'], TRUE); | ||
|
|
There was a problem hiding this comment.
@seanmadsen can you further minimize it to
$params = array_merge($params, array(
'id' => CRM_Utils_Array::value('premium', $ids),
'image' => CRM_Utils_String::simplifyURL(CRM_Utils_Array::value('image', $params, ''), TRUE),
'thumbnail' => CRM_Utils_String::simplifyURL(CRM_Utils_Array::value('thumbnail', $params, '', TRUE),
'is_active' => FALSE,
'is_deductible' => FALSE,
'currency' => CRM_Core_Config::singleton()->defaultCurrency,
));
??
- Move resizeImage() from CRM_Contribute_Form_ManagePremiums to CRM_Utils_File - Make it public static - Don't change anything about how it works
- Clean up CRM_Utils_File::resizeImage() - No major functionality changes - Better docblock - Better error handling - Better variable names
- Refactor CRM_Contribute_BAO_ManagePremiums::add - Don't change any functionality - Improve docblock - Use a $defaults array
- Refactor CRM_Contribute_Form_ManagePremiums::postProcess() - No major changes to functionality - Eliminate deeply nested code (addressing "FIXME" comment) - Split code into multiple functions - Add more comments
- Improve CRM_Contribute_Form_ManagePremiums::formRule() - Require an image file, if "upload" is chosen - Remove the commented-out code for a rule which required images to be within a certain size - Minor refactoring to improve code clarity
This change is the crux of CRM-20821. Before this change, editing a premium product would automatically break the image URL. After this change, premium products can be edited while retaining the image URL. Functionality is preserved which changes the URL to a local URL when possible. This commit adds two new Utils functions (along with tests) to handle the logic of changing a supplied URL to a local URL.
Make CRM_Utils_File::resizeImage() preserve image aspect ratio by default.
monishdeb
force-pushed
the
CRM-20821_CRM-20938
branch
from
60e595f
to
312c781
Compare
monishdeb
force-pushed
the
CRM-20821_CRM-20938
branch
from
312c781
to
bb80d2f
Compare
|
@colemanw @eileenmcnaughton I have reviewed the PR and updated the PR with some additional fix. Ensured that there is no unintended regression due to refactoring. Can you please merge this PR? |
|
@monishdeb I have read your patch and agree that it adds some tidy ups and the lines all look correct. I understand from your comments that you have reviewed all Sean's code & tested it - so I'm happy that this is good to merge. @seanmadsen would be great if you can do a final test against master after merge |
| isset($params['imageOption']) && | ||
| $params['imageOption'] == 'image' && | ||
| empty($files['uploadFile']['name']) | ||
| if (CRM_Utils_Array::value('imageOption', $params['imageOption']) == 'image' |
There was a problem hiding this comment.
The code:
CRM_Utils_Array::value('imageOption', $params['imageOption'])
needs to be changed to
CRM_Utils_Array::value('imageOption', $params)
There was a problem hiding this comment.
Ahh right typo .. sorry about that .. going to create a new PR for that
| ) { | ||
| $errors['uploadFile'] = ts('A file must be selected'); | ||
| } | ||
|
|
||
| // If choosing to use image URLs, then both URLs must be present | ||
| if (isset($params['imageOption']) && $params['imageOption'] == 'thumbnail') { | ||
| if (CRM_Utils_Array::value('imageOption', $params['imageOption']) == 'thumbnail') { |
There was a problem hiding this comment.
The code:
CRM_Utils_Array::value('imageOption', $params['imageOption'])
needs to be changed to
CRM_Utils_Array::value('imageOption', $params)
|
@seanmadsen I have created a separate PR #10761 . Is there anything else I need to fix? |
Before this commit, the URL simplification was performed within the $params array used as the *default* values, so these values got over-written by the submitted values with array_merge(). This is a follow-up to changes made in civicrm#10720 bb80d2f
|
@monishdeb PR #10761 is good in that it addresses my comments above. ✅ In further testing I identified another problem with your changes in bb80d2f. ❌ I am proposing a fix in #10762. I have now completed my testing. Once #10761 and #10762 are merged then I think we can close the book on this little project. |
Issues
This PR addresses both
Details steps to reproduce are provided in both tickets.
Summary
Notes
The reason the images were getting broken is because of ffc4d2d which added logic to "strip protocol and domain from image URLs" but mistakenly ended up being a bit overzealous with its "strip" logic — it would strip path components off the start of the URL no matter what. I improved this logic in 368073f (and added tests to support the new logic).