Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix placement of call to simplifyURL() function #10762

Merged
merged 1 commit into from
Jul 25, 2017

Conversation

seancolsen
Copy link
Contributor

@seancolsen seancolsen commented Jul 25, 2017

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 partial revert of changes made by @monishdeb in bb80d2f in #10720

CC: @colemanw

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
Copy link
Member

monishdeb commented Jul 25, 2017

@seanmadsen I am still confused as the I intended to override the image and thumbnail parameter with its rightful value

 'image' => CRM_Utils_String::simplifyURL(CRM_Utils_Array::value('image', $params, ''), TRUE),

As here it means either 'image' value will be set or else '' is taken and return the simplifyURL(...) for it. And as per the current patch it does the same thing or may be not. In either case I am happy to merge this PR :)

@seancolsen
Copy link
Contributor Author

Try these steps:

  1. Edit a premium product
  2. Upload a new image
  3. Save
  4. Click edit again and look at the value of "Image URL"

With your code, for "Image URL" I get

http://dmaster/sites/default/files/civicrm/persist/contribute/mug_tall_c496d4675c0a1c0637ce24f7f9227f2e_full.jpg

With my code, for "Image URL" I get:

/sites/default/files/civicrm/persist/contribute/mug_tall_8ae8b0f23533e7a9600e5de9958d384e_full.jpg

Why? (I could be wrong here, but this is my assessment)

Your code does 'image' => CRM_Utils_String::simplifyURL(CRM_Utils_Array::value('image', $params, ''), TRUE),... okay, this works fine. But then that value gets overwritten because of the fact that it's inside of array_merge(). So when array_merge() runs, it take the 'image' array element from the second argument of array_merge(), which is $params.

@monishdeb
Copy link
Member

monishdeb commented Jul 25, 2017

That's why we are simplifyingURL for those two attributes outside the array_merge. I see now .. silly me :( . Did a quick check on my local and it worked fine. Tagging with [merge on pass]

@eileenmcnaughton eileenmcnaughton merged commit 587f648 into civicrm:master Jul 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants