-
-
Notifications
You must be signed in to change notification settings - Fork 827
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
dev/core#4487 Alternative option: Fix paymentBlock url concat #27037
dev/core#4487 Alternative option: Fix paymentBlock url concat #27037
Conversation
Thank you for contributing to CiviCRM! ❤️ We will need to test and review the PR. 👷 Introduction for new contributors
Quick links for reviewers |
The issue associated with the Pull Request can be viewed at https://lab.civicrm.org/dev/core/-/issues/4487 |
I like the idea of the & before not after - makes sense - will see if @sebalis can check it out |
I’ve looked at the way this PR solves the issue and I like it much more. This file is now in use on our productive 5.58.1 instance and I’ve verified that contributions work with and without the extra URL parameter (language=en). Of course by using this file I have also inherited the other changes made since 5.58.1, which mainly seem to be QA plus extra checks with a condition called isBackOffice, and I hope that this won’t break anything if it’s used in 5.58.1 – a question unrelated to this PR, admittedly, but if you have any comment I’d be glad to know, otherwise I’ll risk it. In any case, please merge this PR (I have already closed mine) while still allowing me to be credited as a contributor; seems fair to me as I pointed out the issue and where it needed to be solved. ;-) |
Thanks @sebalis. I think you're probably OK with the new version of the file, since the changes are just reducing PHP warnings, but no promises. Sorry, this was merged before I saw your note, so I can't add you as a co-author any more. |
Many thanks for the advice about the file, @larssandergreen – and not to worry, it seems that the contributor’s credit issue has been very kindly taken care of :-) |
Overview
Alternate for #27031. I think this is little more readable and maintainable.
Before
Funky JS plus Smarty concatenation breaks if additional URL params are present
After
Less funky concatenation. Also simplified the ampersand situation a bit.