Skip to content

[RFC] Always enable JSON support in php 8.0 #5495

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

[RFC] Always enable JSON support in php 8.0 #5495

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 5 additions & 13 deletions ext/json/config.m4
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,11 @@
PHP_ARG_ENABLE([json],
[whether to enable JavaScript Object Serialization support],
[AS_HELP_STRING([--disable-json],
[Disable JavaScript Object Serialization support])],
[yes])

if test "$PHP_JSON" != "no"; then
AC_DEFINE([HAVE_JSON],1 ,[whether to enable JavaScript Object Serialization support])
Copy link
Member

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.

Copy link
Member

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

Copy link
Contributor

@hikari-no-yume hikari-no-yume May 3, 2020 •
edited
Loading

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!

Copy link
Contributor Author

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).

Copy link
Contributor Author

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).

phpinternalsbook/PHP-Internals-Book#43

Copy link
Member

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Committed as 5609701.

Copy link
Contributor

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();
}

Copy link
Contributor

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

Copy link
Contributor

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


dnl HAVE_JSON is always 1 as of php 8.0 and the constant will be removed in the future.
dnl Note that HAVE_JSON was never defined for Windows builds (see config.w32)
AC_DEFINE([HAVE_JSON],1 ,[whether to enable JavaScript Object Serialization support])
PHP_NEW_EXTENSION(json,
json.c \
json_encoder.c \
json_parser.tab.c \
json_scanner.c,
$ext_shared,, -DZEND_ENABLE_STATIC_TSRMLS_CACHE=1)
PHP_INSTALL_HEADERS([ext/json], [php_json.h php_json_parser.h php_json_scanner.h])
PHP_ADD_MAKEFILE_FRAGMENT()
PHP_SUBST(JSON_SHARED_LIBADD)
fi
PHP_INSTALL_HEADERS([ext/json], [php_json.h php_json_parser.h php_json_scanner.h])
PHP_ADD_MAKEFILE_FRAGMENT()
28 changes: 12 additions & 16 deletions ext/json/config.w32
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,22 +1,18 @@
// vim:ft=javascript

ARG_ENABLE("json", "JavaScript Object Serialization support", "yes");
EXTENSION('json', 'json.c', false /* never shared */, "/DZEND_ENABLE_STATIC_TSRMLS_CACHE=1");

if (PHP_JSON != "no") {
EXTENSION('json', 'json.c', PHP_JSON_SHARED, "/DZEND_ENABLE_STATIC_TSRMLS_CACHE=1");

if (!FSO.FileExists("ext/json/json_scanner.c")) {
STDOUT.WriteLine("Generating ext/json/json_scanner.c");
STDOUT.WriteLine(execute(PATH_PROG("re2c") + " -t ext/json/php_json_scanner_defs.h --no-generation-date -bci -o ext/json/json_scanner.c ext/json/json_scanner.re"));
}
if (!FSO.FileExists("ext/json/json_parser.tab.c")) {
STDOUT.WriteLine("Generating ext/json/json_parser.tab.c");
STDOUT.WriteLine(execute(PATH_PROG("bison") + " --defines -l ext/json/json_parser.y -o ext/json/json_parser.tab.c"));
}
if (!FSO.FileExists("ext/json/json_scanner.c")) {
STDOUT.WriteLine("Generating ext/json/json_scanner.c");
STDOUT.WriteLine(execute(PATH_PROG("re2c") + " -t ext/json/php_json_scanner_defs.h --no-generation-date -bci -o ext/json/json_scanner.c ext/json/json_scanner.re"));
}
if (!FSO.FileExists("ext/json/json_parser.tab.c")) {
STDOUT.WriteLine("Generating ext/json/json_parser.tab.c");
STDOUT.WriteLine(execute(PATH_PROG("bison") + " --defines -l ext/json/json_parser.y -o ext/json/json_parser.tab.c"));
}

ADD_SOURCES(configure_module_dirname, "json_encoder.c json_parser.tab.c json_scanner.c", "json");
ADD_SOURCES(configure_module_dirname, "json_encoder.c json_parser.tab.c json_scanner.c", "json");

ADD_MAKEFILE_FRAGMENT();
ADD_MAKEFILE_FRAGMENT();

PHP_INSTALL_HEADERS("ext/json/", "php_json.h php_json_parser.h php_json_scanner.h");
}
PHP_INSTALL_HEADERS("ext/json/", "php_json.h php_json_parser.h php_json_scanner.h");