-
Notifications
You must be signed in to change notification settings - Fork 7.7k
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
[RFC] Always enable JSON support in php 8.0 #5495
Conversation
[yes]) | ||
|
||
if test "$PHP_JSON" != "no"; then | ||
AC_DEFINE([HAVE_JSON],1 ,[whether to enable JavaScript Object Serialization support]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would keep the define. There may be other extensions (not bundled with PHP) which check for this. It costs essentially nothing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We didn't leave these for ext/hash
and actively removed similar defines for other builtin extensions like ext/pcre
, ext/spl
about a year or so ago, so if this RFC passes I think it should follow the same style
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes things needlessly annoying for PECL extensions etc that need to support older versions. We kept around the TSRMLS macros for a few versions, we can keep this around. Not all precedents are good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes things needlessly annoying for PECL extensions etc that need to support older versions. We kept around TSRMLS, we can keep this around. Not all precedents are good!
It also makes it obvious that the extension authors are able to use json functionality unconditionally, and discourages people from manually patching php 8 to disable json and have HAVE_JSON set to 0.
I'm having a hard time finding how often/where HAVE_JSON
is actually used - I suppose downloading the top 1000 pecl releases is possible but haven't done that before. Then again, lack of recommendations or tutorials on how to use config.m4 may contribute to that.
https://github.com/php-memcached-dev/php-memcached/blob/v3.1.5/config.m4 (checks for the existence of the header)
https://github.com/mongodb/mongo-php-driver/blob/master/config.m4 uses PHP_ADD_EXTENSION_DEP(mongodb, json)
We made TSRMLS a no-op in 7.0 and removed it from 8.0, but it's much more frequently used than HAVE_JSON, so the numbers of lines of code affected is very different
Altogether, though - whether or not HAVE_JSON is there isn't very important to me, but having a clear policy on when to remove macros would help. I'll probably end up restoring the macro since multiple people have raised objections (and keep the rfc to a single vote).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then again, lack of recommendations or tutorials on how to use config.m4 may contribute to that.
I checked if there were recommendations published since the last time I looked at a config.m4 with dependencies - I had similar questions back in 2017, and solved those for my use case by looking at what other extensions were doing in config.m4 and config.w32, which required a few revisions to fix on rarer build setups (e.g. statically compiled an extension on windows).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Jan-E for pointing this out. I think the proper fix would be:
ext/json/config.w32 | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ext/json/config.w32 b/ext/json/config.w32
index 9d7881118d..255f278772 100644
--- a/ext/json/config.w32
+++ b/ext/json/config.w32
@@ -1,7 +1,7 @@
// vim:ft=javascript
EXTENSION('json', 'json.c', false /* never shared */, "/DZEND_ENABLE_STATIC_TSRMLS_CACHE=1");
-
+PHP_JSON="yes";
ADD_SOURCES(configure_module_dirname, "json_encoder.c json_parser.tab.c json_scanner.c", "json");
ADD_MAKEFILE_FRAGMENT();
I need to check phpize builds; if these are good, I'll commit that fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Committed as 5609701.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I apply this commit to PHP 8.0.0RC2, I am running into another Jscript error with mongodb in ext/mongodb:
N:\php-sdk\php80dev\configure.js(8470, 3) Microsoft JScript runtime error: Path not found
Line 8470 in configure.js is the 7th line in the current config.w32 of mongodb:
https://github.com/mongodb/mongo-php-driver/blob/master/config.w32#L7
configure_module_dirname = condense_path(FSO.GetParentFolderName('N:\\php-sdk\\php80dev\\ext\\mongodb\\config.w32'));
// vim:ft=javascript
function mongodb_generate_header(inpath, outpath, replacements)
{
STDOUT.WriteLine("Generating " + outpath);
var infile = FSO.OpenTextFile(inpath, 1);
var outdata = infile.ReadAll();
infile.Close();
for (var key in replacements) {
var replacement = replacements[key];
if (typeof replacement === 'string') {
replacement = replacement.replace(/"/g, '\\"');
}
outdata = outdata.replace(new RegExp('@' + key + '@', 'g'), replacement);
}
var outfile = FSO.CreateTextFile(outpath, true);
outfile.Write(outdata);
outfile.Close();
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forget my last comment for now. It was a missing libbson. The inpath was pointing to:
mongodb/src/libmongoc/src/libbson/src/bson/bson-config.h.in
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With a properly installed ext/mongodb compilation runs into the expected errors, because the mongodb extension is not fit yet for PHP 8.
ext/ds with a ADD_EXTENSION_DEP('ds', 'json')
compiles fine after 5609701
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Windows changes look good to me
FYI @oerdnj @kaplanlior (listed as the upstream debian maintainers for the php7.4 packages according to https://packages.ubuntu.com/focal/php-json) Will moving I'm working on the RFC https://wiki.php.net/rfc/always_enable_json (the implementation is this PR) (I've assumed there's a process in place to deal with extensions being moved into core, since extensions such as reflection and hash have been moved into core in the past. https://wiki.php.net/rfc/permanent_hash_ext) Also, are there other package maintainers that you'd suggest I contact? Aside: For Mac OS, Homebrew seems to keep JSON with its default of compiling it statically. (no overrides change that, e.g. |
FYI @vakartel https://git.alpinelinux.org/aports/tree/community/php7/APKBUILD https://gitlab.alpinelinux.org/alpine/aports/issues/8299 |
No problems, not at all. |
No issue with downstream (RPM) |
Currently, it's possible to disable the json extension with `./configure --disable-json` (for historical reasons that no longer apply). However, JSON is widely used in many use cases - web sites, logging output, and as a data format that can be used to share data with many applications and programming languages, so I'd personally find it useful if it was always enabled. Examples of where this would be useful: - For internal classes to be able to implement `JsonSerializable` which currently requires a hard dependency on the JSON extension. - For PHP users to publish single-file scripts that use json_encode and json_decode and don't require polyfills or less readable var_export output. (polyfills are less efficient and may have issues with recursive data structures) - So that php-src's own modules, tools and test cases can start using JSON if it's a good choice for encoding a value. (same for PECLs) https://wiki.php.net/rfc/jsond mentions that in PHP 5, > The current Json Parser in the json extension does not have a free license > which is a problem for many Linux distros. > This has been referenced at Bug #63520. > That results in not packaging json extension in the many Linux distributions. Starting in php 7.0 with the switch to jsond, It looks like licensing is no longer an issue. Changes: - Remove all flags related to JSON such as `configure --disable-json` - Require that JSON be compiled statically instead of as a shared library Examples of uses of JSON in various distros (backwards incompatible changes such as changing packaging are typically reserved for major versions, and 8.0 is a major version) - JSON is required by `php-cli` or `php` in ubuntu: https://packages.ubuntu.com/focal/php/ - The php-json package has to be installed separately from the PHP binary in Fedora repos.
a1e8e06
to
81c94a6
Compare
Late comment: see php-ds/ext-ds#162 for a config.w32 fix. |
Currently, it's possible to disable the json extension with
./configure --disable-json
(for historical reasons that no longer apply).However, JSON is widely used in many use cases - web sites, logging output,
and as a data format that can be used to share data with many applications
and programming languages,
so I'd personally find it useful if it was always enabled.
Examples of where this would be useful:
JsonSerializable
which currently requires a hard dependency on the JSON extension.
(e.g. DateTime has custom JSON serialization behavior, but is unable to implement JsonSerializable normally)
json_decode and don't require polyfills or less readable var_export output.
(polyfills are less efficient and may have issues with recursive data
structures)
if it's a good choice for encoding a value. (same for PECLs)
https://wiki.php.net/rfc/jsond mentions that in PHP 5,
Starting in php 7.0 with the switch to jsond,
It looks like licensing is no longer an issue.
Changes:
HAVE_JSON
will not be removed in this PR, and will always be1
.(Except on Windows, where it seems like it was never defined in any previous PHP release)
configure --disable-json
Examples of uses of JSON in various distros
(backwards incompatible changes such as changing packaging are typically
reserved for major versions, and 8.0 is a major version)
JSON is compiled statically into PHP in Ubuntu 20?.(mentioned by someone else on an email thread but I'm not able to confirm this, I may be looking at the wrong repository since I see php-json in https://packages.ubuntu.com/focal/php/)from the PHP binary in Fedora repos.
./buildconf --force; ./configure; make
) compiles JSON statically by defaultMentioned in https://externals.io/message/109783 (Moving json extension to core?)
https://wiki.php.net/rfc/always_enable_json (draft)
It turns out that php-json is still a separate package in https://packages.ubuntu.com/focal/php/
https://github.com/php/php-src/blob/master/ext/json/json.c is licensed under the PHP License 3.01, like php's core.
In an Ubuntu 20.04 image, installing
php7.4-cli
orphp
pulls inphp7.4-json
as a shared library during the installation.