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

[NFC] If statement is always true #16122

Merged
merged 1 commit into from
Dec 22, 2019

Conversation

demeritcowboy
Copy link
Contributor

@demeritcowboy demeritcowboy commented Dec 19, 2019

Overview

This line is always true. It uses boolean OR and has been that way since it was added 8 years ago.

   if ($config->userFramework != 'Joomla' ||
     $config->userFramework != 'WordPress'
   ) {

Before

TRUE

After

TRUE

Technical Details

To be super-picky, if userFramework was numeric 0 the line would be false, but userFramework shouldn't be numeric 0.

Comments

If you look at the diff ignoring whitespace it's easier to see the change. The rest is indentation.

While the line was likely intended to be boolean AND, the scope of this PR is just to remove a line that does nothing and has been doing nothing for 8 years. There is another related line but even when that is removed it doesn't make report permissions fully work for Joomla, likely because of yet more lines elsewhere. This PR doesn't address that.

@civibot
Copy link

civibot bot commented Dec 19, 2019

(Standard links)

@civibot civibot bot added the master label Dec 19, 2019
@MegaphoneJon
Copy link
Contributor

I haven't done an r-run of the code (can do so later today) but I reviewed the code and agree that @demeritcowboy is correct.

For a bonus cleanup - there's an ampersand on an object that can disappear too, but obviously not a blocker.

@demeritcowboy
Copy link
Contributor Author

Thanks. Yes you're right that ampersand could go.

@demeritcowboy
Copy link
Contributor Author

jenkins retest this please

@demeritcowboy demeritcowboy changed the title If statement is always true [NFC] If statement is always true Dec 20, 2019
@eileenmcnaughton
Copy link
Contributor

I see @MegaphoneJon was going to r-run & didn't get back to it. I looked at the code & agree with the 2 of you that it is impossible for the IF to be true so I'm OK merging based our our 3 readings of the code - I'm a little unsure how we would r-run it tbh

@eileenmcnaughton eileenmcnaughton merged commit df8f605 into civicrm:master Dec 22, 2019
@demeritcowboy demeritcowboy deleted the always-true branch December 25, 2019 02:06
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