PHP < 5.4.0 compatibility for "--dump-schema" #109
Conversation
|
+1 |
|
@hakre would you mind reviewing this as well? Thanks for the help 🍻 |
| @@ -218,7 +218,8 @@ $refResolver = new JsonSchema\RefResolver($retriever); | |||
| $refResolver->resolve($schema, $urlSchema); | |||
|
|
|||
| if (isset($arOptions['--dump-schema'])) { | |||
| echo json_encode($schema, JSON_PRETTY_PRINT) . "\n"; | |||
| $options = version_compare(PHP_VERSION, '5.4.0', '>=') ? JSON_PRETTY_PRINT : 0; | |||
There was a problem hiding this comment.
You want to check if sth you want to use is available for you.
Therefore please check that sth, not sth other. When you want to have JSON_PRETTY_PRINT you should test for it, not for some PHP_VERSION. Remember that one may want to run it under hhvm, and it has it's own problem with constants (they forgot to create some of them).
Ref: https://github.com/FriendsOfPHP/PHP-CS-Fixer/blob/1.7/Symfony/CS/Utils.php#L30
There was a problem hiding this comment.
and, btw, PHP_VERSION_ID is hell faster than calling version_comapre ;)
|
here you go! |
|
@keradus Thanks for the feedback and changing my thought process for things like this! I have made the update to check if the constant is defined instead of checking the PHP version. |
|
@siwinski: I suggest you rebase your branch on top of master. Let me know if you need help with that. Otherwise go into your branch and type: In case you run into a conflict you don't want to fix / fear, you can always type in then to touch safe ground again. And even if you mess things up greater than that there is always the git reflog as a safety net. Rebasing will put your changes on top of master as if you've been doing your work on the latest version. This keeps conflicts low and the PR small. Ping me if you've got any questions. |
siwinski
force-pushed
the
pr-php-lt-5-4-0-compat
branch
from
fb78dad to
aaa9ada
Compare
|
@hakre LOL... I had originally done that but then couldn't remember if GitHub would close out this original PR or lost the original inline comments so I went with the merge. Rebased. |
|
@siwinski: Cheers! Have you seen the comment on the FULL STOP vs. COMMA operator I just created with much love? Or is it lost now? Edit: Just in case, I found a link to it: siwinski@fb78dad#commitcomment-10476310 |
siwinski
force-pushed
the
pr-php-lt-5-4-0-compat
branch
from
aaa9ada to
1fc065c
Compare
|
@hakre I'm so used to my internal team's process of keeping all commits instead of squashing and merging instead of rebasing that I forgot how to deal with things "outside" :) I've done it plenty and pushed the squash up. |
|
@siwinski: Well if I would have bothered so much and as it's on the same line and really superfluous, I would have done that. In the sense of care-taking and as we've got the luxury of an extensive review here at this moment in time. But it's no show-stopper. Your PR, your choice. I won't run gaga if you wouldn't . Thanks for the fix btw. |
PHP < 5.4.0 compatibility for "--dump-schema"
|
@hakre sorry... I meant to come back and make that change but then this PR got accepted |
|
@siwinski: nothing to feel sorry for. And also you can make a new PR if you like. |
JSON_PRETTY_PRINT available since PHP 5.4.0