dev/core#4509 Fix link conditionals when using current_domain for dom… #27093
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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/4509 |
8f8cc62 to
46a3c46
Compare
colemanw
reviewed
Aug 19, 2023
| @@ -574,6 +574,15 @@ private function checkLinkCondition(array $item, array $data): bool { | |||
| } | |||
| return TRUE; | |||
| } | |||
| // Convert the conditional value of 'current_domain' into an actual value that filterCompare can work with | |||
| if (substr($item['condition'][0], 0, 9) === 'domain_id' && $item['condition'][2] === 'current_domain') { | |||
There was a problem hiding this comment.
This bit (substr($item['condition'][0], 0, 9) === 'domain_id') looks like stringey-guesswork. I don't think it would work with joins, or with any field that doesn't happen to be named domain_id. IMO just omit it.
There was a problem hiding this comment.
yeh I think i was just trying to limit when this could be done but your comment makes sense updated
colemanw
reviewed
Aug 19, 2023
| @@ -574,6 +574,15 @@ private function checkLinkCondition(array $item, array $data): bool { | |||
| } | |||
| return TRUE; | |||
| } | |||
| // Convert the conditional value of 'current_domain' into an actual value that filterCompare can work with | |||
| if (substr($item['condition'][0], 0, 9) === 'domain_id' && $item['condition'][2] === 'current_domain') { | |||
| if (strpos($item['condition'][0], 'label') !== FALSE) { | |||
There was a problem hiding this comment.
Shouldn't this be
Suggested change
| if (strpos($item['condition'][0], 'label') !== FALSE) { | |
| if (str_ends_with($item['condition'][0], ':label')) { |
There was a problem hiding this comment.
Updated to that, I didn't know about the str_ends_with
There was a problem hiding this comment.
It's new in PHP8, but Civi includes shims for older php versions so we're allowed to use it :)
46a3c46 to
22ec0d5
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…ain_id columns
Overview
This fixes an issue with link conditionals where if you specify current_domain for the value it doesn't work because current_domain is passed through to the actual comparison function where it doesn't work because it hasn't been properly substituted.
Before
Link conditionals don't work with current_domain value
After
Link conditionals work with current_domain value
ping @colemanw