Fix test suite on travisci #292
Conversation
|
good job, i tried to fix the test in my pull request 👍 #284 |
|
We can remove support for PHP 5.3 if this helps cleaning up the tests, this should help avoid many of these changes. Would you mind doing this in your PR? |
|
@schmittjoh sure I can do it :) |
custom stub. Skipped whitelist test for php version which changed behavior. Remove support for php 5.3
|
@schmittjoh done :) |
| @@ -73,7 +73,6 @@ public function testPropertyIsCollectionOfObjectsWithAttributeAndValue() | |||
|
|
|||
| /** | |||
| * @expectedException JMS\Serializer\Exception\InvalidArgumentException | |||
| * @expectedExceptionMessage The document type "<!DOCTYPE author [<!ENTITY foo SYSTEM "php://filter/read=convert.base64-encode/resource=XmlSerializationTest.php">]>" is not allowed. If it is safe, you may add it to the whitelist configuration. | |||
There was a problem hiding this comment.
Could you elaborate this change, and the next one?
There was a problem hiding this comment.
This is related with https://bugs.php.net/bug.php?id=67081 seems that php change internalSubset property of DOM so this message is valid until 5.5.13 and 5.4.29 We can see that changes was merged into 5.5.13 and 5.4.29 in the php changelog http://php.net/ChangeLog-5.php
There was a problem hiding this comment.
If we switch to setExpectedException, it should still be possible to assert the error message depending on the PHP version, no? Something like
$expectedMessage = $this->getInvalidDocTypeMessage('....');
$this->setExpectedException('class', $expectedMessage);
As this is security related, I think we should do a bit more here than just verifying the exception type.
There was a problem hiding this comment.
yeah I think that with setExpectedException it should be possible. I will try to update this PR at the end of this weekend.
empty object. Fixed issue schmittjoh#277 Rely on schmittjoh#292
| @@ -32,7 +32,8 @@ | |||
| "symfony/translation": "~2.0", | |||
| "symfony/validator": "~2.0", | |||
| "symfony/form": "~2.1", | |||
| "symfony/filesystem": "2.*" | |||
| "symfony/filesystem": "2.*", | |||
| "phpunit/phpunit": "4.1.*" | |||
There was a problem hiding this comment.
4.1.* is a bad idea. 4.1 is not the latest stable.
and you could simply continue to rely on the version installed on Travis IMO
There was a problem hiding this comment.
I think relying on a fixed version is generally a good thing to make things a bit more stable.
There was a problem hiding this comment.
but it requires downloading 14 or 15 more packages for each build, making the composer installation slower while composer is still there.
And anyway, if you decide to add the dependency, it should be ~4.1, not 4.1.*, so that a composer update can select newer minor versions when they are released (which would be the case now that 4.2 is stable since a few weeks)
|
I suggest splitting it in 2 separate PRs:
|
|
Final part of this is located in #375. Tests are passing now! =) |
|
@schmittjoh I think you can just close this PR now ) |
|
Yes, thanks :) |
Bascially this PR fixed build at the travisci.
Just be aware about few things:
$thisusage in anonymous functions.Give me know if this PR is acceptable, if yes I can squash commits.