-
Notifications
You must be signed in to change notification settings - Fork 536
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
Introduced PHP 8 support; removed support for PHP 5.6 + 7.0 #383
Conversation
* bumped SYMFONY_PHPUNIT_VERSION from 7.5 to 8.5 * extended ElementXRef::equals allow equals checks in PHP 8+; otherwise tests would fail because 5_0 != 5
because tests failed with errors like > PHP Fatal error: Declaration of Tests\Smalot\PdfParser\Integration\ParserTest::setUp(): Tests\Smalot\PdfParser\Integration\void must be compatible with Tests\Smalot\PdfParser\TestCase::setUp(): Tests\Smalot\PdfParser\void in /home/runner/work/pdfparser/pdfparser/tests/Integration/ParserTest.php on line 279 Ref: https://github.com/smalot/pdfparser/pull/383/checks?check_run_id=1633732760#step:6:45
This comment has been minimized.
This comment has been minimized.
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 am not sure about the following addition
If that fix the issue, I'm ok with it.
It fixes the test for specific parameters, but I am not sure if it covers it 100%. Can you give me a hint what range of parameter values |
I've no idea... |
Nope, sorry, I have no clue either! |
❗ We should wait with a merge until further information from #104. Its about a memory leak, but some still use PHP 5.6. I would like to sort it out before we cut off PHP 5.6 + 7.0 support. |
I learned from semver/semver#468 that dropping support for an supported environment is also considered a breaking change. It means that we would immediately go to 1.0, which is not a bad thing. I wanna try out something before we do that, maybe we can get PHP 8 and keep PHP 5.6/7.0. |
/** | ||
* In case $value is a number and $this->value is a string like 5_0 | ||
* | ||
* Without this if-clause code like: | ||
* | ||
* $element = new ElementXRef('5_0'); | ||
* $this->assertTrue($element->equals(5)); | ||
* | ||
* would fail (= 5_0 and 5 are not equal in PHP 8.0+). | ||
*/ | ||
if ( | ||
true === is_numeric($value) | ||
&& true === \is_string($this->getContent()) | ||
&& 1 === preg_match('/[0-9]+\_[0-9]+/', $this->getContent(), $matches) | ||
) { | ||
return (float) ($this->getContent()) == $value; | ||
} | ||
|
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.
@smalot Do you see any issues with that addition?
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.
Can you remind me when does this method is called and why we try to validate equality between "5" and "5_0" ?
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 you remove this code, assertion in testEquals
of ElementXRefTest.php will fail in PHP 8.0.
* Removed Simple PHPUnit and PHP-CS-Fixer from composer.json * Simple PHPUnit was used to allow us run tests for PHP 5.6-7.4 * It adds additional complexity and is not needed anymore. Thats why I replaced it with PHPUnit itself. * Created a new folder "dev-tools" with a composer.json. that is the new home for PHPUnit, PHPStan and PHP-CS-Fixer * Simplified workflows a little bit by using Makefile commands sometimes. * Added Makefile to make it easier for developers to run tests or other tools. * Added DEVELOPER.md which contains information about the dev-tools we use. * Added PHPStan to dev-tools, because it is used in a workflow Reasons for removing tools from root composer.json: * PHPUnit and PHP-CS-Fixer should not be a dependency for the library, because they are only relevant for development. (e.g. https://github.com/FriendsOfPHP/PHP-CS-Fixer#installation) * Moving them to a separate space makes our tooling easier, e.g. in our CI workflow we had to remove PHP-CS-Fixer to allow a run without problems * PHPStan was already used separately which supports my decision to move the rest too. * Dev-Tools like PHPUnit have their own requirements and we had problems in the past, which required additional work. For instance in PHPUnit 8: they added return type void to a few methods like setUp, which forced us to decide if we keep PHP 5.6 or we are stuck on PHPUnit 7.5 and can't use PHP 8. Source: https://phpunit.de/announcements/phpunit-8.html * IMHO having dev tools in a separate place at least reduces the danger that a tool affects the library.
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.
What the benefit of moving dev tools deps in a dedicated folder?
Co-authored-by: Jérémy Benoist <j0k3r@users.noreply.github.com>
I am sorry I wanted to add the reasons right away, but was interrupted. Here is a copy of my commit message:
|
Updated my initial post with a few TODOs. Semantic Versioning is not clear what happened if a supported environment (in this case PHP 5.6 + 7.0) is dropped. I found a few issues discussing this, like semver/semver#526 (comment). I would follow the argument and create a new major version (= 1.0.0). 0.18.2 would be the last |
it used outdated PHPUnit command
Hi @k00ni |
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 need to understand why ElementXRef contains this fix
/** | ||
* In case $value is a number and $this->value is a string like 5_0 | ||
* | ||
* Without this if-clause code like: | ||
* | ||
* $element = new ElementXRef('5_0'); | ||
* $this->assertTrue($element->equals(5)); | ||
* | ||
* would fail (= 5_0 and 5 are not equal in PHP 8.0+). | ||
*/ | ||
if ( | ||
true === is_numeric($value) | ||
&& true === \is_string($this->getContent()) | ||
&& 1 === preg_match('/[0-9]+\_[0-9]+/', $this->getContent(), $matches) | ||
) { | ||
return (float) ($this->getContent()) == $value; | ||
} | ||
|
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.
Can you remind me when does this method is called and why we try to validate equality between "5" and "5_0" ?
Thanks @k00ni, everything seems to be good for me |
@smalot Just to be sure we didn't forget something. What about https://github.com/smalot/pdfparser/pull/383/files/f595ac26308b167c3a02c3a6a77abd653c47b61e#r592171760 (my addition to |
Because there is no activity/progress in referenced issues I am inclined to merge this PR and prepare a major release. |
Updated 2021-03-10:
5_0
!=5
(see TODOs below)dev-tools
, which contains a separatecomposer.json
file. A detailed explanation why I did that can be found in Introduced PHP 8 support; removed support for PHP 5.6 + 7.0 #383 (comment)TODOs: