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

Fix continue / break warning under PHP 7.3 #228

Merged
merged 1 commit into from
Nov 22, 2018

Conversation

ejegg
Copy link
Contributor

@ejegg ejegg commented Nov 6, 2018

PHP 7.3 has started to issue warnings when a continue statement is
inside a switch and interpreted as a break
"continue" targeting switch is equivalent to "break".
Did you mean to use "continue 2"?
In this case, it looks like 'continue 2' is the right way to go.

@civibot
Copy link

civibot bot commented Nov 6, 2018

(Standard links)

@civibot civibot bot added the master label Nov 6, 2018
@ejegg
Copy link
Contributor Author

ejegg commented Nov 6, 2018

strange, api_v3_JobProcessMailingTest::testConcurrency.testConcurrency is failing with all its datasets, but it's hard to tell what this commit could have done. The other 1000+ tests are passing, though!

@eileenmcnaughton
Copy link
Contributor

@ejegg that test had a known issue after a server rebuild....

I agree with your analysis on continue 2 being correct. It's a dog of a construct though!

if ($val & DB_DATAOBJECT_STR) {
    $ret[$key] = Validate::string($this->$key, VALIDATE_PUNCTUATION . VALIDATE_NAME);
    continue;
}

if (($val & DB_DATAOBJECT_INT):
   $ret[$key] = Validate::number($this->$key, array('decimal'=>'.'));
   continue;
}

would save some braincells IMHO

@eileenmcnaughton
Copy link
Contributor

test this please

1 similar comment
@seamuslee001
Copy link
Contributor

test this please

PHP 7.3 has started to issue warnings when a continue statement is
inside a switch and interpreted as a break
  "continue" targeting switch is equivalent to "break".
  Did you mean to use "continue 2"?
In this case, the switch statement was weird anyway, so I've
replaced it with a couple of if statements to do the same thing.
@eileenmcnaughton
Copy link
Contributor

merging per previous discussion on this code (above)

@eileenmcnaughton eileenmcnaughton merged commit 8e62f54 into civicrm:master Nov 22, 2018
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