The 2026 Drupal Business Survey is open. Your response will help shape the future of the Drupal business ecosystem.Just like core in #2853503: Remove all assert('string') calls from Drupal core because deprecated in PHP 7.2, the JSON API module will need to update its assert() calls.
| Comment | File | Size | Author |
|---|---|---|---|
| #2 | 2920892-2.patch | 981 bytes | wim leers |
Comments
Comment #2
wim leersComment #3
wim leersThis is trivial enough that I can commit it right away.
I'm surprised that there's only one
assert():)Comment #4
wim leersComment #5
e0ipso@Wim Leers, did you commit that in the end? I don't see the commit notification in the comments.
Does this require a PHP version dependency? In other words, does asserting via an expression (and not a function) work with all php5 versions supported by D8?
Comment #6
wim leersYes: http://cgit.drupalcode.org/jsonapi/commit/?id=4c13013.
d.o commit messages in issues appear to be broken.
Yes, it works on all versions. The difference is that on PHP 5, the logic in the
assert()call will now be executed 100% of the time, instead of only whenassert()s are enabled inphp.ini. This is a decision made by the PHP team.Comment #7
e0ipsoDoes this have a performance impact on production servers running PHP5?
Comment #8
wim leersYes, since the assertions will now run 100% of the time.
Comment #9
e0ipsoOne of the reasons to add these as assertions was that they'd only run during development. If that is not true anymore, then we should come up with an alternative approach before tagging the next release.
Comment #10
wim leersI agree, and I hadn't thought of that when I committed this, it seems obvious now. It was late on Friday. This change was necessary in any case, for PHP 7.2 compatibility. 7.2.0 is being released on November 30, so this change was necessary in the next JSON API release anyway.
It really sucks that the PHP team is forcing this :(
I was opening a new issue for that, but then I realized … that the validation is only happening if the
justinrainbow/json-schemadev dependency is installed. Which won't be the case on production sites anyway.So perhaps the thing is that this should not use
assert()at all, it should run always when the dev dependency is installed?Comment #12
e0ipsoI don't think we can ensure that the presence of that popular PHP library means development mode. It could have been added by a different contrib.
Comment #13
wim leersYou’re right.
Let’s create a new issue then, to only run the assert if assertions are enabled. /me waves fist at PHP team.
Let’s make sure to add a todo to remove that condition when D8 only supports PHP 7.