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

Remove $_REQUEST passed into CRM_Utils_Request::retrieve NFC #17377

Conversation

artfulrobot
Copy link
Contributor

This is a subset of #17324

A lot of code called CRM_Utils_Request::retrieve or CRM_Utils_Request::retrieveValue with the wrong value type for it's $method which is supposed to be a String, but in dozens of places code was passing the $_REQUEST Array. The retrieve method is and was coded with a switch that checks on strings 'POST' and 'GET', and in all other cases (e.g. an array!), it defaulted to $_REQUEST - so serendipity has served us well! In these cases I have simply removed that param since it's the default behaviour anyway.

This PR only contains such changes and as a result should make no difference; it's a NFC

@civibot
Copy link

civibot bot commented May 21, 2020

(Standard links)

@@ -30,7 +30,7 @@ public function buildQuickForm() {
public static function commonBuildQuickForm($self) {
$contactId = CRM_Utils_Request::retrieve('cid', 'Positive', $self);
if (!$contactId) {
$contactId = CRM_Utils_Request::retrieve('cid', 'Positive', CRM_Core_DAO::$_nullObject, FALSE, NULL, $_REQUEST);
$contactId = CRM_Utils_Request::retrieve('cid', 'Positive');
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What does this code even do?

Line 31 fetches 'cid' from POST, failing that, GET, failing that from $self->get('cid').

Then if that fails, we try again, but without $self->get('cid')? But we know that will yield the same results anyway, no?

Copy link
Member

Choose a reason for hiding this comment

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

These four lines starting with if (!$contactId) { look pointless to me.

@artfulrobot artfulrobot force-pushed the artfulrobot-remove-incorrect-calls-to-request-retrieve branch from 2499a0c to ab6f007 Compare May 25, 2020 11:52
@artfulrobot
Copy link
Contributor Author

I've done more of a clean up, removing other default params.

@mattwire
Copy link
Contributor

@artfulrobot There is now a test failure, I don't know if it is related?

@artfulrobot
Copy link
Contributor Author

Interesting, I'll take a look

@artfulrobot
Copy link
Contributor Author

Jenkins please test this.

(that failed test passes repeatably on my buildkit)

@mattwire
Copy link
Contributor

@artfulrobot Still failing :-(

@artfulrobot
Copy link
Contributor Author

What do you do when you can't reproduce a test fail locally?! Any tips?

@artfulrobot artfulrobot force-pushed the artfulrobot-remove-incorrect-calls-to-request-retrieve branch from ab6f007 to 1be22cf Compare May 26, 2020 16:42
@artfulrobot
Copy link
Contributor Author

Jenkins please test this.

If at first you don't succeed, try try again...

@mattwire
Copy link
Contributor

Let's merge this quick before it fails again ;-)

@mattwire mattwire merged commit 49efc06 into civicrm:master May 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants