[Utility] [CS] Use simplified symfony version comparison and minor cs fixes #939
Conversation
robfrawley
added
the
State: Reviewing
| } | ||
|
|
||
| return version_compare($kernel, $vernum, $operator); | ||
| return version_compare(Kernel::VERSION_ID, sprintf("%d%'.02d%'.02d", $major, $minor ?: 0, $patch ?: 0), $operator); |
There was a problem hiding this comment.
Why are you padding the minor and patch components? The previous code didn't do this and I don't see a reason why it would be needed.
Nice use of ?: by the way 💯
There was a problem hiding this comment.
Regarding the padding, this isn't using the same comparison value; hence the change. More specifically, the padding is required because we're now comparing to the Kernel::VERSION_ID constant, which would be assigned the value int(20801) for version 2.8.1. This requires we change the input parameters of, say int(2), int(3), null(), into 20300.
Also, I love ?: 👍
There was a problem hiding this comment.
By the way, I found that the :? is actually not required, as null is cast to 0 automatically, but I found this made the usage more clear, and I don't like relying on PHP's internal type casting behavior.
There was a problem hiding this comment.
Ah, my mistake, I thought you were comparing against VERSION, not VERSION_ID.
That being said I can't find any documentation on the VERSION constant, so I'm not sure how it's constructed but from what I've seen thusfar numeric comparison should suffice.
There was a problem hiding this comment.
As for simple numeric comparison, that isn't possible due to the nature of this method signature: it allows passing in the comparison operator, hence the usage of version_compare.
The VERSION constant isn't a bad choice, but it is sometimes followed by a string value like -dev and -beta and while I know version_compare can deal with those, it just doesn't seem as clear to me. From my 100% unscientific polling of other projects, usage of VERSION_ID seems to be the most often used way to version detect, which was another reason I'd chosen it originally.
This change actually originated from a bug in symfony/symfony#master where the kernel's VERSION_MAJOR was accidentally set to 0 causing our tests to fail: I settled on VERION_ID after looking at what others used.
Do you feel that using VERSION instead of VERSION_ID would add any value?
There was a problem hiding this comment.
Using VERSION instead of VERSION_ID is more in line with what the code previously did, but other projects use VERSION_ID and that works for them so there is some empirical evidence that it works I'd say go for it.
Of course, feature detection (class_exists, method_exists et al) is always better than kernel version comparison, but that's beside the point of this PR.
There was a problem hiding this comment.
Yeah, feature detection is of course preferable. Thankfully, this class is no longer required (for either version or feature detection) on the 2.x branch. You may also feel better (as I do) that version detection is only used once or twice in the production codebase; the majority of calls occur in the test suite.
:-)
There was a problem hiding this comment.
Lol, that does make me feel better 😃
robfrawley
changed the title
This changes the internal behavior of
SymfonyFramework::kernelVersionCompare()by using theKernel::VERSION_IDconstant instead of the separated version numbers (major, minor, patch). The resulting API behavior is identical. To see only these changes, reference commit 39d820.The second commit contains automated CS fixes from a PHP CS Fixer run.
Also of interest, I added the internal docblock to the
SymfonyFrameworkclass, which I suggest we more regularly adopt for code we don't intend to be used by consuming projects.