Add support for php7 and get all tests running #53
Conversation
andrerom
changed the title
| @@ -59,7 +59,7 @@ public function load(array $configs, ContainerBuilder $container) | |||
| $processor = new ConfigurationProcessor($container, 'ezpublish_legacy'); | |||
| $processor->mapConfig( | |||
| $config, | |||
| function (array &$scopeSettings, $currentScope, ContextualizerInterface $contextualizer) { | |||
| function (array $scopeSettings, $currentScope, ContextualizerInterface $contextualizer) { | |||
There was a problem hiding this comment.
Is this something that could break on PHP 5.x?
There was a problem hiding this comment.
Don't see how, the variable is not modified inside the closure.
There was a problem hiding this comment.
Yep, just wanted to confirm.
Might fail on a few things.
Not needed in this case as it is not modified, and value given.
andrerom
force-pushed
the
test-on-php7
branch
2 times, most recently
from
0d95f40
to
087efc7
Compare
andrerom
changed the title
|
review ping @emodric @lolautruche |
| @@ -47,6 +47,10 @@ public function onBuildKernelWebHandler(PreBuildKernelWebHandlerEvent $event) | |||
| $request = $event->getRequest(); | |||
| $uriPart = array(); | |||
|
|
|||
| // @todo Remove when, if ever, legacy does not use deprecated constructor names | |||
| // triggers initial autoload of eZSiteAccess and sileces deprecation warning | |||
| @eZSiteAccess::TYPE_DEFAULT; | |||
There was a problem hiding this comment.
I don't get this...
As for the comment, you just need to silent E_DEPRECATED errors in php.ini
There was a problem hiding this comment.
We can, but for symfony 3.0 support we actually want to have deprecation errors make test fail on other things. Hence the selective silencing just when loading legacy classes, as we kind of want to know about anything else.
There was a problem hiding this comment.
but for symfony 3.0 support we actually want to have deprecation errors make test fail on other things
E_DEPRECATED !== E_USER_DEPRECATED
😉
There was a problem hiding this comment.
true, ok I'll see if I get that running then.
There was a problem hiding this comment.
fixed, but by just switching error level for this test only, want to get other deprecation errors if there are any. constructor is perhaps the only we shouldn't do anything about for BC.
andrerom
force-pushed
the
test-on-php7
branch
2 times, most recently
from
90d6969
to
e7b7cd1
Compare
| "matthiasnoback/symfony-dependency-injection-test": "^1.0", | ||
| "phpunit/phpunit": "~4.7", | ||
| "mikey179/vfsStream": "~1.1.0", | ||
| "mockery/mockery": "^0.9@dev" |
There was a problem hiding this comment.
There was a problem hiding this comment.
Okay, but isn't ^0.9 enough?
There was a problem hiding this comment.
it can be, but then we won't pull in those fixes as it will use the tag.
There was a problem hiding this comment.
It will pull 0.9.5, that's not enough?
There was a problem hiding this comment.
does not sound like you checked the link, but removed it
There was a problem hiding this comment.
does not sound like you checked the link, but removed it
I did, but the link shows diff between 0.9 and 0.9.5 versions. ^0.9 will pull in 0.9.5, so I'm not sure what the issue is :)
|
Aside from inline comment, +1. |
| @@ -42,12 +42,12 @@ public function purgeAll() | |||
| $this->gatewayCachePurger->purgeAll(); | |||
| } | |||
|
|
|||
| public function purgeForContent($contentId) | |||
| public function purgeForContent($contentId, $locationIds = array()) | |||
There was a problem hiding this comment.
Isn't this a breaking change? If so, we should require eZ Kernel >=6.3.2|>=6.4.2 (when 6.4.2 is released) in composer.json, right?
There was a problem hiding this comment.
yes, but adding optional argument won't break the contract. In retrospective it was the kernel that broke the contract here.
So besides the fact that kernel should not have done this change, I think this fix is ok as is, it will work on older kernels also.
There was a problem hiding this comment.
So besides the fact that kernel should not have done this change, I think this fix is ok as is, it will work on older kernels also.
Agreed.
|
looks ok to you as well now @lolautruche ? EDIT: Hmm, failing now, might be because of the d0a0c33 change edi asked me to do. |
|
+1 Weird that it doesn't fail with PHP 5.4 |
…tor names as it will most likely never be changed
…r to dump config here
|
Found it, we allow SensioDistributionBundle 5.0 now for work towards Sf 3.0 support, it supports PHP 5.5+, and it does not contain Configurator as needed by LegacyBridge. Added it to composer.json as it is a clear requirement. Thanks for reviews, merging :) |
Might fail on a few things.passingNote: PHP7 might never be fully supported with legacy, this just makes sure it can be used for dev use, and is a first step towards getting legacy running on php7.