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 unnecessary transformation of upgrade msg severity #18182

Merged
merged 1 commit into from
Aug 19, 2020

Conversation

colemanw
Copy link
Member

Overview

Removes unnecessary code that ultimately does nothing.

Before

Severity string coming back from the server would be put through some complex transformation that didn't ultimately do anything.

After

The original string was fine.

Technical Details

The constructor it's being passed to works just fine with with string input, and the strings are already correct.

@civibot
Copy link

civibot bot commented Aug 17, 2020

(Standard links)

@civibot civibot bot added the master label Aug 17, 2020
@eileenmcnaughton
Copy link
Contributor

@colemanw this is interesting because @mattwire & I were talking about using a string 'Completed' vs using a number that needs to be converted to that string vs using a true Constant & here you are switching from a constant to a string. Is there an advantage in using constants?

@colemanw
Copy link
Member Author

Constants prevent spelling errors, and make the list of accepted strings discoverable. Other than that I can't think of one.

In this case we are getting a string back from an external service. The string matches up with the values of the constants used elsewhere in this file.

@colemanw
Copy link
Member Author

Note that the constructor of this function is flexible about what you pass it. You can either pass a string or an integer and it will transform as-needed. Elsewhere in the file you'll see the constructor being passed a constant for that param (the value of which is a string).

@eileenmcnaughton
Copy link
Contributor

I feel like I've seen breaks on constants before too - not relevant here though - when code is moved or whatever

@demeritcowboy
Copy link
Contributor

When I run this it works but they're all blue, but I see the same thing on another site without the patch - was that a change elsewhere to tone down the scariness? These aren't messages I usually look at but I vaguely remember something more in-your-face.

Untitled

Otherwise, on the constants topic, the way I read the history of this is that originally it was all PSR constants, which are strings, but then it was updated to use ints internally to make thresholds easier, and secretly the constructor would also take ints. But why this mapping was added here seems unclear, other than that pingback itself does not use PSR constants, they just happen to match the strings, so maybe it was added as a layer of insulation, but it's not needed currently.

@colemanw colemanw merged commit e9f6c2b into civicrm:master Aug 19, 2020
@colemanw colemanw deleted the severity branch August 19, 2020 21:22
@colemanw
Copy link
Member Author

Thanks @demeritcowboy for the review.
I think the reason it's blue is because they are not critical updates. It would change to a scarier color if they were security patches.

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