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.

Sign up for GitHub

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

Jump to bottom

[REF] Reconcile CRM_Utils_System::getUrlPath and CRM_Utils_System::currentPath #17068

Merged
merged 2 commits into from
Apr 29, 2020
Merged

[REF] Reconcile CRM_Utils_System::getUrlPath and CRM_Utils_System::currentPath #17068

merged 2 commits into from
Apr 29, 2020

Conversation

colemanw
Copy link
Member

@colemanw colemanw commented Apr 13, 2020 •
edited
Loading

Overview

Code refactoring to make looking up the current URL more consistent.

Before

  • 2 functions that did the same thing (retrieve current url).
  • Lots of code using neither function but doing it the hard way directly accessing $_GET.

After

  • Deprecated CRM_Utils_System::getUrlPath.
  • Switched code to use the non-deprecated CRM_Utils_System::currentPath.
  • Refactored code accessing $_GET directly to use the helper function.

@civibot
Copy link

civibot bot commented Apr 13, 2020

(Standard links)

@civibot civibot bot added the master label Apr 13, 2020
colemanw added 2 commits April 13, 2020 17:20
@colemanw
Deprecate redundant function CRM_Utils_System::getUrlPath
f31f885 
Both functions do the same thing; deprecating getUrlPath in favor of the older currentPath.
@colemanw
REF - Cleanup [userFrameworkURLVar] to use CRM_Utils_System::currentP…
13594d5 
…ath()
seamuslee001
seamuslee001 reviewed Apr 13, 2020
View reviewed changes
CRM/Report/Utils/Report.php
if (CRM_Utils_Rule::positiveInteger($arg[3])) {
return $arg[3];
}
if (isset($arg[3]) && $arg[1] == 'report' && $arg[2] == 'instance' && CRM_Utils_Rule::positiveInteger($arg[3])) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@colemanw what if there is no 2 key? I guess that would be covered by the first isset right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. It's always going to be a non-associative array, so if [3] is set then so are [2] and [1] and [0].

@kcristiano
Copy link
Member

@colemanw I'll do some r-run on the various configurations that caused us url and path trouble in 5.23 on Joomla and WP. Will likely be next week.

@mattwire
Copy link
Contributor

@colemanw What is not obvious from the description is which one is deprecated! It is obvious looking at the code but would help reviewers :-)

@colemanw
Copy link
Member Author

Thanks @kcristiano for testing.
@mattwire I've updated the description.

@kcristiano
Copy link
Member

@colemanw I've done r-run on WP and Joomla and have not hit any errors.

I think this should be merged so we can have in the next RC

@colemanw colemanw merged commit 21586d7 into civicrm:master Apr 29, 2020
@colemanw colemanw deleted the currentPath branch April 29, 2020 13:58
agileware-justin pushed a commit to agileware/org.civicrm.dataprocessor that referenced this pull request Jul 2, 2020
@colemanw
Switch to non-deprecated function CRM_Utils_System::currentPath
1f58a82 
See civicrm/civicrm-core#17068
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seamuslee001 seamuslee001 seamuslee001 left review comments

Assignees
No one assigned
Labels
master
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@colemanw @kcristiano @mattwire @seamuslee001