-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Update requirement to PHP 7.2 #1951
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
Conversation
|
|
||
| if (in_array($type, array(Header::AUTO, Header::FIRST, Header::EVEN))) { | ||
| $index = count($collection); | ||
| $index = is_countable($collection) ? count($collection) : 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.
Why is_countable check? Properties $this->headers and $this->footers is array.
| // Populate style and register to global Style register | ||
| $style = $listTypeStyles[$this->listType]; | ||
| $numProperties = count($properties); | ||
| $numProperties = is_countable($properties) ? count($properties) : 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.
Why is_countable check? Variable $properties is array.
| PHPWord requires the following: | ||
|
|
||
| - PHP 5.3.3+ | ||
| - A [supported version of PHP](https://www.php.net/supported-versions.php) |
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's ambiguous. I would rather give a specific version number as before - PHP 7.2+
| "tecnickcom/tcpdf": "6.*", | ||
| "mpdf/mpdf": "5.7.4 || 6.* || 7.*", | ||
| "mpdf/mpdf": "7.* || 8.*", | ||
| "rector/rector": "^0.8", |
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.
You only used rector library for conversion. Now, in my opinion, not needed as require-dev, including the following rector.php file. Remove it.
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.
In think this could be useful to add other rules as well. See Rector as a phpcsfixer-like tool.
Thanks for the feedback, I'll work on it as soon as your PR has been merged.
| $name = str_replace("\x00", "", substr($data, 0, $nameSize)); | ||
|
|
||
|
|
||
| $this->props = (array) $this->props; |
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.
Why this cast? The problem is in the missing definition of variable for class. Just add private $props = [];.
| if (!empty($elements)) { | ||
| foreach ($elements as $file => $media) { | ||
| if (count($media) > 0) { | ||
| if ((is_countable($media) ? count($media) : 0) > 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.
Same as above, variable $media is array.
| */ | ||
| const PDF_RENDERER_DOMPDF = 'DomPDF'; | ||
| const PDF_RENDERER_TCPDF = 'TCPDF'; | ||
| const PDF_RENDERER_TCPDF = \TCPDF::class; |
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.
This contant is not a class designation. Class \TCPDF may not exist.
| const ZIPARCHIVE = 'ZipArchive'; | ||
| const PCLZIP = 'PclZip'; | ||
| const OLD_LIB = 'PhpOffice\\PhpWord\\Shared\\ZipArchive'; // @deprecated 0.11 | ||
| const ZIPARCHIVE = \ZipArchive::class; |
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.
Class \ZipArchive is part of ext-zip extension. In production install PHPWord does not require ext-zip. Class \ZipArchive may not exist.
| @@ -0,0 +1,22 @@ | |||
| <?php | |||
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.
Remove file, explained above.
Description
I've updated the required PHP to 7.2 Please note that this version is already out of active support: https://www.php.net/supported-versions.php. It has security support until 30 Nov 2020.
I've used Rector to do some small updates to the codebase. I haven't added any typing (type hints, return types) yet.
Checklist:
composer run-script check --timeout=0and no errors were reportedShould be merged after #1946.
Fixes #1948.