Skip to content
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

PHP Compactor removes attributes when Phar is being generated on PHP < 8.0 #567

Closed
jrfnl opened this issue Oct 10, 2021 · 4 comments · Fixed by #568
Closed

PHP Compactor removes attributes when Phar is being generated on PHP < 8.0 #567

jrfnl opened this issue Oct 10, 2021 · 4 comments · Fixed by #568

Comments

@jrfnl
Copy link
Contributor

jrfnl commented Oct 10, 2021

Bug report

Question Answer
Box version 3.13.0@275b091
PHP version PHP < 8.0 for Phar generation / PHP 8.1 for using generated Phars
Platform with version Any
Github Repo All users, including this repo itself just as well.

Context

Attributes were introduced in PHP 8.0, but few projects (so far) were using them.

However, PHP 8.1 has added type declarations to all PHP native interfaces and classes.

For userland code implementing these interfaces or extending the classes, this means that they:

  • Either have to add the type declarations to the affected methods as well;
  • Or if this is not (yet) possible due to the minimum supported PHP version for the project being below the PHP version in which the type can be used (mixed is PHP 8.0, void is PHP 7.1 etc), an attribute can be added to silence the deprecation warning which would otherwise be thrown in PHP 8.1 and higher.

In effect, this means that a lot more projects will now suddenly start using attributes.

Example:

// Original code:
class MyJson implements JsonSerializable {
    public jsonSerialize() {}
}

// Updated for compatibility with PHP 8.1 by adding the type:
class MyJson implements JsonSerializable {
    public jsonSerialize(): mixed {}
}

// Updated for compatibility with PHP 8.1 by adding the attribute:
class MyJson implements JsonSerializable {
    #[\ReturnTypeWillChange]
    public jsonSerialize() {}
}

The Problem

The PHP Compactor - KevinGH\Box\Compactor\Php -, which is included with Box, does not account for PHP 8.0+ attributes.

In PHP < 8.0, attributes are tokenized as comments and the compactor blindly removes those, while for Phar files which need to be compatible with PHP 8.1, those attributes need to stay in the code to prevent the deprecation notices.

Solution

The PHP Compactor needs to be made compatible with PHP 8.0 in regards to attributes.

Stop-gap solution until a release has been tagged containing a proper solution

For the time being, projects which need to generate Phar files which are compatible with PHP 8.1 will need to:

  1. Add "check-requirements": false to their box.json configuration to prevent the notices coming from Box itself showing.
    Those notices were already fixed in PHP 8.1: fix two deprecation warnings #557, but a) that fix has not been included in a tagged release yet and b) even if it were, the box.json.dist config for Box itself includes the PHP compactor and with the Phar being generated on PHP 7.3, the attributes would be stripped from the generated Box Phar file.
  2. Either needs to be generated on PHP 8.0 in which the attributes are no longer tokenized as comments;
    Or if the Phar generation is done on PHP < 8.0, the PHP compactor needs to be turned off/removed for the time being until the compactor code in Box has been fixed.

Loosely related to #557 as the attributes introduced in that PR will ALSO be removed if a Phar is generated (including for Box itself) on PHP < 8.0.

@jrfnl
Copy link
Contributor Author

jrfnl commented Oct 10, 2021

I've opened PR #568 to address this issue.

jrfnl added a commit to jrfnl/PHP-Parallel-Lint that referenced this issue Oct 10, 2021
For the time being, until box-project/box#567 has been fixed:
1. Disable the addition of the requirement checker from Box as the version included in the last release of Box is not compatible with PHP 8.1.
2. Change the PHP version used to generate the Phar to PHP 8.0 to get round the problem the PHP compactor has with attributes.
    Note: the generated Phar file should still be compatible with all supported PHP versions.
jrfnl added a commit to jrfnl/PHP-Parallel-Lint that referenced this issue Oct 11, 2021
For the time being, until box-project/box#567 has been fixed:
1. Disable the addition of the requirement checker from Box as the version included in the last release of Box is not compatible with PHP 8.1.
2. Change the PHP version used to generate the Phar to PHP 8.0 to get round the problem the PHP compactor has with attributes.
    Note: the generated Phar file should still be compatible with all supported PHP versions.
jrfnl added a commit to jrfnl/PHP-Parallel-Lint that referenced this issue Oct 11, 2021
For the time being, until box-project/box#567 has been fixed:
1. Disable the addition of the requirement checker from Box as the version included in the last release of Box is not compatible with PHP 8.1.
2. Change the PHP version used to generate the Phar to PHP 8.0 to get round the problem the PHP compactor has with attributes.
    Note: the generated Phar file should still be compatible with all supported PHP versions.
jrfnl added a commit to jrfnl/PHP-Parallel-Lint that referenced this issue Oct 11, 2021
For the time being, until box-project/box#567 has been fixed:
1. Disable the addition of the requirement checker from Box as the version included in the last release of Box is not compatible with PHP 8.1.
2. Change the PHP version used to generate the Phar to PHP 8.0 to get round the problem the PHP compactor has with attributes.
    Note: the generated Phar file should still be compatible with all supported PHP versions.
jrfnl added a commit to jrfnl/PHP-Parallel-Lint that referenced this issue Oct 11, 2021
For the time being, until box-project/box#567 has been fixed:
1. Disable the addition of the requirement checker from Box as the version included in the last release of Box is not compatible with PHP 8.1.
2. Change the PHP version used to generate the Phar to PHP 8.0 to get round the problem the PHP compactor has with attributes.
    Note: the generated Phar file should still be compatible with all supported PHP versions.
jrfnl added a commit to jrfnl/PHP-Parallel-Lint that referenced this issue Oct 11, 2021
For the time being, until box-project/box#567 has been fixed:
1. Disable the addition of the requirement checker from Box as the version included in the last release of Box is not compatible with PHP 8.1.
2. Change the PHP version used to generate the Phar to PHP 8.0 to get round the problem the PHP compactor has with attributes.
    Note: the generated Phar file should still be compatible with all supported PHP versions.
jrfnl added a commit to jrfnl/PHP-Parallel-Lint that referenced this issue Dec 1, 2021
For the time being, until box-project/box#567 has been fixed:
1. Disable the addition of the requirement checker from Box as the version included in the last release of Box is not compatible with PHP 8.1.
2. Change the PHP version used to generate the Phar to PHP 8.0 to get round the problem the PHP compactor has with attributes.
    Note: the generated Phar file should still be compatible with all supported PHP versions.
grogy pushed a commit to php-parallel-lint/PHP-Parallel-Lint that referenced this issue Dec 1, 2021
For the time being, until box-project/box#567 has been fixed:
1. Disable the addition of the requirement checker from Box as the version included in the last release of Box is not compatible with PHP 8.1.
2. Change the PHP version used to generate the Phar to PHP 8.0 to get round the problem the PHP compactor has with attributes.
    Note: the generated Phar file should still be compatible with all supported PHP versions.
@jrfnl
Copy link
Contributor Author

jrfnl commented Dec 16, 2021

@theofidry Any idea when a new version of Box will be tagged including this fix ?

@theofidry
Copy link
Member

@jrfnl
Copy link
Contributor Author

jrfnl commented Dec 16, 2021

Thanks @theofidry !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants