Update libmagic patch

[petk]

This syncs the libmagic (file library) patch with recent changes:

• Remove HAVE_SIGNAL_H from libmagic

  Following left over since 5f89157

  This syncs also libmagic patch with proper fixes of whitespaces errors,
  versions, line numbers, etc.

• Remove HAVE_LOCALE_H check

  This was removed via e06836a and
  file library update 622b10f.

• Remove HAVE_STRERROR

  Already removed via e6a6017 and
  in the libmagic 5.37.

[weltling]

Thanks for the PR. Could we please omit changes in the bundled libmagic files?

Thanks.

[petk]

These changes of the bundled files are in the libmagic 5.37 release already... So, this is a sync as how the libmagic 5.37 should look like actually.

• trailing whitespace cause error in applying patch
• signal.h, locale.h, strerror.h checks (those HAVE_* symbols) are already removed across the php-src and the libmagic 5.37 release. These are part of C89 standard and they are available everywhere today already.

So, if I omit changes in the bundled files, then patch makes no sense? Or what do you mean here?

[weltling]

What I mean, is that porting a new libmagic version is a time consuming process. Having a patch with unrelated customizations makes that porting one more bit harder :) There will be enough places to fail when it's applied to the next libmagic version.

It is not our codebase after all, even if it needs to be patched. If there's no bugfix, there's no real reason to touch the bundled code.

Also i think that the change to the configure.ac might break some external code that includes php.h, as the macros are not defined anymore. IMO there is no real benefit doing that in first place, but for sure these defines should be kept for BC. Core vs. external extensions and libs are different matters, need to be careful.

Thanks.

[petk]

What I mean, is that porting a new libmagic version is a time consuming process. Having a patch with unrelated customizations makes that porting one more bit harder...

That's what this patch does. To make things easier to patch in the future. Without applying these changes (at least the libmagic related ones) it will be more difficult to patch libmagic 5.38, for example. These changes are all in the upstream https://github.com/file/file libmagic (a.k.a file library/tool)... already. To be more clear here: These aren't unrelated changes. These are syncs with the upstream version 5.37. Easiest to understand them is to apply them on the libmagic fork or a branch... With this patch (if I'm not mistaken) patching the 5.37 version is correct and the current patched version in the php-src is not correct. Well, I guess now I'll need to recheck this.

Regarding the impact on the PHP extensions: When would then be the right time for this step according to you? I guess we can leave few of those left overs in the php config header file, yes, but also there are only few left now... These are pre-C89 (not even C89) stuff we're talking about here.

[weltling]

These are syncs with the upstream version 5.37.

Nice then. Sure some piece could have been missed in the port. Just the links in the description point to the php-src, not to the upstream.

The configure.ac checks - well, they could be kept forewer, as they don't hurt at all. Or at least defer as much as possible, to the next major, etc? The topic seems not directly akin to the libmagic stuff, while ofc would affect if the defines are still respected upstream.

Thanks.

[petk]

All these have been applied so far in the 5.37 version:
https://github.com/file/file/pulls?q=is%3Apr+is%3Aclosed+author%3Apetk

Except for the ancient memory.h file issue - which will be removed in the 5.38 release (but this is not important for the php's libmagic.patch file since it is not in it)...

Keeping such symbols for ever in php config header files or windows one for the same matter is actually problematic... I'll get back to you with a more clear understanding how minimal this impact actually is...

[petk]

Hello, so the results of the usages in the open source extensions:

• HAVE_LOCALE_H: 2 (apc, imagick) - nothing that couldn't get fixed
• HAVE_SIGNAL_H: 1 (mysql) - also nothing much problematic
• HAVE_STRERR: 0

Scanned so far: 160 extensions...

Considering the results of these usages, I still think that it needs to be made more clear, that these symbols are prehistoric checks and don't need to be used like this anymore. So, all good to start removing these defines already in PHP 7.4. I'm also not so sure how really portable extensions are with relying on the symbols defined during the configuration of the build of PHP. Will it really work always if the extension is built on the system separately from PHP? Like cross compilations, shared uninstalled extensions etc. With these 3 symbols for example, yes, it would be ok, but extensions might rely on other symbols that depend on the system libraries, and behavior...

[weltling]

Great research, thanks! For imagick it seems to be an issue that might render previous releases unusable, as per here https://github.com/Imagick/imagick/blob/bb93a3da072c2243934fe4629c0f47a12407da9a/php_imagick_defs.h#L100. Imagick is very popular https://pecl.php.net/package-stats.php?pid=76&rid=&cid=12 and there seems to be still a lot of activity on the older releases. Other than that, it seems to be fine. Perhaps remove teh locale one in master, and the others in 7.4 would be the middle way?

Also ping @Danack :)

Thanks.

[weltling]

@petk seems you'll need to rebase, i had to update it due to another issue. While on that, i was able to check your change to libmagic and it is fine. I would suggeest the following

• recreate the libmagic.patch and move on on it
• separate the global config changes into another pR
• keep the locale.h define, we pinged the imagic maintainer, but the compatibility breach is bad for the existing releases

One more note - i haven't noticed there was a script to automate the patch creation. I'm not sure it's quite correct, because the diff has to be done to the configured source, but probably even better to the built source. Perhaps it's something you or @nikic could look at, i'll do as well when having time.

Thanks.

[petk]

Added. How to check this pull request:

mkdir patching-libmagic
cd patching-libmagic
git clone git://github.com/php/php-src
cd php-src
git checkout PHP-7.4
cd ..
git clone git://github.com/file/file
cd file
git checkout -b php-5-37 FILE5_37
autoreconf -f -i
./configure
make
cd src
wget https://raw.githubusercontent.com/petk/php-src/patch-file/ext/fileinfo/libmagic.patch
patch < libmagic.patch
cd ..
git diff --name-only | xargs -I{} cp {} ../php-src/ext/fileinfo/libmagic/
cd ..
cd php-src
git diff

Redacted: one git checkout was missing.

[petk]

About the other changes mentioned to separate them, they now depend on this pull request, so then it's best to open them once this is merged... Hopefully this can be done on some reasonable quick time, so this gets to the PHP-7.4... Thanks.

[weltling]

@petk nice level of detail :) Looks good AFM, thanks for cleaning the missed parts.

Thanks.

[petk]

@weltling this is then good to merge?

cc @nikic

thanks.

[weltling]

@petk sure. I didn't follow up as looked like you'd be able to merge yourself. Please merge when you've time.

Thanks.

[petk]

Applied. Thanks. Worth noting that locale.h is now still checked and defined for PHP-7.4 but will be removed in PHP 8 then.

Otherwise, I'm not sure if this is necessary since the imagick library still needs to be patched anyway due to using HAVE_SPL symbol (cc @Danack). I think this is the only extension I could find on the whole to use the locale.h check from PHP... Also, pull request has been sent to Imagick/imagick#291

[weltling]

@petk nice you filed the PR to imagick. With locale and other C99 changes - C99 is enforced in master, with lower we're still on C89 formally. Having this going a bit finer route for a smoother BC seems not wrong therefore. As well as with the other changes that might affect external extensions.

Thanks.