PHP 8.0 & 8.1 #266
PHP 8.0 & 8.1 #266
Conversation
lib/plugins/sfDoctrinePlugin/lib/test/sfTesterDoctrine.class.php
Outdated
Show resolved
Hide resolved
|
Sorry to be late for the review ! |
with code from FriendsOfSymfony1/symfony1#266 to bring Propel sfYaml*.php files in sync
|
Who is able to merge this pr? As far as I can tell, all requested changes have been fixed. |
|
@Tybaze I would wait for FriendsOfSymfony1/doctrine1#85 to be merged. In the meantime: can you squash some of the latest commits? (the "apply fix from thePanz", for example, those can go into a single "Apply code-style fixes" together with the other cs-fixes... wdyt? yes, I know, not a good commit message anyway ) :) |
|
Rebased ! Removed credits in the comments ^^ |
| @@ -1003,10 +1003,10 @@ public function getCurrentCacheKey() | |||
| { | |||
| $cacheKey = $this->routing->getCurrentInternalUri(); | |||
|
|
|||
| if ($getParameters = $this->request->getGetParameters()) | |||
| if ($cacheKey && $getParameters = $this->request->getGetParameters()) | |||
There was a problem hiding this comment.
Is this meant to be here?
we will not consider the parameters if the $cacheKey is an empty string... do we have a test for this?
There was a problem hiding this comment.
getCurrentInternalUri() can return null (on a sfPatternRouting with a null $currentRouteName)
And in that case on PHP8.1 it will trigger a warning while trying to concatenated the &/?
but on PhP <8.0, it will works and create a $cacheKey containing only the query string.
So this modification will break a empty currentRouteName, as it will not cache the Get parameters anymore
let me fix it
There was a problem hiding this comment.
Sorry there is no warning on concatenate on null ...
There was a problem hiding this comment.
The issue is on the strpos($cacheKey, '?') — I get 3 times the error below for every page request for which the route doesn't exist (e.g. 404 case) that has query parameters in it.
PHP Deprecated: strpos(): Passing null to parameter #1 ($haystack) of type string is deprecated in [...]/vendor/friendsofsymfony1/symfony1/lib/view/sfViewCacheManager.class.php on line 1008
There's no warning if there are no query parameters.
I suppose a more innocuous solution will be to cast the $cacheKey on stripos(), although I'm not sure it makes sense to be generating a cacheKey for a route that doesn't exist and only if it has query parameters. But perhaps in the context of this PR, it's best to avoid any behavior changes.
There was a problem hiding this comment.
As the sfExecutionFilter can handle a null value
$uri = $this->context->getViewCacheManager()->getCurrentCacheKey();
if (null !== $uri && $this->context->getViewCacheManager()->hasActionCache($uri))
So null is a valid value for getCurrentCacheKey() ,
Adding if($cacheKey...) check would break existing feature of enable caching if there are some GET parameters
So I revert this fix in order to keep the existing behavior.
We should need some test to check each possible $cacheKey value
Rebase Done
There was a problem hiding this comment.
We need to at least cast cacheKey on strpos($cacheKey, '?') or there are deprecation warnings on php8.1 with unnamed routes with parameters.
There was a problem hiding this comment.
Thanks @mentalstring , I applied your suggestion, by casting the $cacheKey to (string).
It should keep the previous behavior and fix the PHP8.1 warning.
Rebased !
@thePanz is it ok for you ?
Just for fun, I run some benchmark on casting to (string).
On PHP8.1 I got a ~10% overhead to the strpos() call with the cast.
Tybaze
force-pushed
the
compat_php8.1
branch
2 times, most recently
from
e3d5748 to
716a01f
Compare
|
Ping, everything should have been fixed or at least be commented. How about merging and releasing this thing? Thanks for your effort, @Tybaze! |
sfException> fileExcerpt file can be null, and PHP 8.1 do not allow null on is_readable() sfBrowser> Move sf_test conf before getContext, because getContext can throw some sfException, which will raise some printStackTrace, hidden by another Exception "header already sent ..." lime.php> some trace can have no "file" (internal methods call) lime.php> handle_exception can handle Error/Throwable, not avaialble under php7.2: remove typing sfTestFunctionalBase> can throw exception Fix Select Unit test NewActivePendingExpired. DomDocument on recent php return a list of values, not concatened ones. Fix SessionStorage UnitTest. sfSessionStorage could not be restarted. Flag $sessionStarted as false when shutdown to avoid error during unit test which can start several sfSessionStorage Storage need to be shutdown to avoid: PHP Warning: session_name(): Session name cannot be changed when a session is active
… "error" value uncaught exception does not populate error field
…ated, return an integer less than, equal to, or greater than zero
ValueError: fread(): Argument FriendsOfSymfony1#2 ($length) must be greater than 0 TypeError: count(): Argument #1 ($value) must be of type Countable|array Foo Given Use Symfony Polyfill symfony/polyfill@d330c00 TypeError: count(): Argument #1 ($value) must be of type Countable|array, string given Do not use Symfony pollyfill, is_array is enought
… null, $accept = false) must be compatible with sfPearRest::downloadHttp($url, $lastmodified = null, $accept = false, $channel = false)
thePanz
force-pushed
the
compat_php8.1
branch
4 times, most recently
from
81a427e to
6da2c17
Compare
8.0.0 hour is no longer optional. 8.0.0 minute, second, month, day and year are nullable now.
Numeric strings ending with whitespace ("42 ") will now return true. Previously, false was return instead.
Use same fix has main maintened Yaml lib :
symfony/yaml@4152e36
+ performance improvement
symfony/yaml@2b5f2ae
When $previousIndent was equal to 0, and $matches['indent'] = " " PHP(before 8.0)> (0 != " ") ==> false PHP8.0+ > (0 != " ") ==> true In order to keep "false" value we avoid "0" as a valid value. More details here why here: https://www.php.net/manual/en/migration80.incompatible.php
thePanz
force-pushed
the
compat_php8.1
branch
from
6da2c17 to
f7a8509
Compare
/bin/ls can be reported as: - application/x-sharedlib - application/x-executable - application/x-pie-executable
|
I applied some additional changes @Tybaze , and fixed the last failure for the ValidatorFile GitHub actions are now happy for PHP v7.4, v8.0 and v8.1 |
thePanz
force-pushed
the
compat_php8.1
branch
from
f7a8509 to
1e9b188
Compare
|
Thanks and kudos to @Tybaze for making this possible! Thanks @thirsch @mentalstring too 👍 |
PHP 8.0 & 8.1 compatibility
[No enhancement - please consider to use Symfony5+ for your new Project]
Target : Maintains maximum backward compatibility down to 5.3.
Backport some fixes from PR #265, but I do not use "internal method return type", because use of "mixed" return type is required.
But "mixed" keyword is not available before PHP 7.4, use anotation instead. 5f2b710
A lot of currently LTS Unix Distro are still running PHP 7.2 / 7.3 currently - and the goal of this repo is to maintain a sf1 running in most setup.
Tests
Great thanks to @alquerci for #262, rebasing with the /test/bin/test dockers tool.
Currently pass all test.
For the moment, to run a successful PHP 8.1 test you should remove E_DEPRECATED or use my last commit for Doctrine1 submodule (Until FriendsOfSymfony1/doctrine1#85 be merged ):
Future
One day PHP 9 will break compatibility with php < 7.4 due to the lacks of "mixed" keywords implementations.
Please consider rewriting your project to another framework, like Symfony5+ if your app do not support at least PHP 7.4.