Symfony 4 support #200
Symfony 4 support #200
Conversation
wachterjohannes
force-pushed
the
symfony-4-support
branch
7 times, most recently
from
0be2b98 to
4b30fcb
Compare
wachterjohannes
force-pushed
the
symfony-4-support
branch
2 times, most recently
from
32fd137 to
5b26275
Compare
|
@dbu did you have an idea why the tests fail with symfony 4.0? there is a strange error with multiple |
|
i don't think its cp commands, but some parser having a problem. maybe the yaml parser of symfony? in version 4 its more strict and does not tolerate some crap data that it tolarated before. can it be that there is some yaml file in this repo that has a "cp" key that is duplicated? |
|
@dbu thats a good hint :) i will check this |
wachterjohannes
force-pushed
the
symfony-4-support
branch
from
5b26275 to
740bb04
Compare
|
@dbu i have found it: https://github.com/phpcr/phpcr-shell/blob/master/src/PHPCR/Shell/Resources/config.dist/alias.yml the cp alias is duplicated and stands for 2 different things! i have tested it asap in a current installation and |
paging @dantleech :-) |
|
@dbu @dantleech as you can see here https://github.com/phpcr/phpcr-shell/pull/200/files#diff-a31654ea4b9244c0b548a8e9c11d0b15L52 the |
sounds good to me, I guess somehow before the first one had precedence. |
|
@dbu @dantleech thanks for your advices - for me the PR is OK i can also re-add the entry with a prefix :) but could you guys take a look at the rest? |
composer.json
Outdated
| "phpcr/phpcr-utils": "~1.2", | ||
| "symfony/finder": "~2.8|~3.4|~4.0", | ||
| "symfony/serializer": "~2.8|~3.4|~4.0", | ||
| "symfony/yaml": "~2.8|~3.4|~4.0", |
There was a problem hiding this comment.
indeed could be 3.0 i will change that here
There was a problem hiding this comment.
sorry, imho this should be 3.4 indeed. all other 3.x versions of symfony are end of life and don't even get security fixes anymore: https://symfony.com/roadmap
wachterjohannes
force-pushed
the
symfony-4-support
branch
2 times, most recently
from
2b91fd6 to
638924c
Compare
|
@dantleech fixed |
There was a problem hiding this comment.
looks good to me.
you moved the suites into the matrix. that reduces the number of builds, but i find the selection a bit confusing. why have the matrix that you have? no cli on 5.6 lowest, no standalone on 7.0 and no cli on 7.1. i am unsue what the difference of those suites is, but if they test distinct things, i'd keep the config separately and live with the many builds. if they all test the same, lets only have lowest and 7.2 do all and have 7.0 and 7.1 only run one single suite.
we should eventually fix the travis config to not hide missing symfony component dependencies, but this is not a new problem so i don't mind.
| @@ -49,7 +49,6 @@ checkin: version:checkin | |||
| ci: version:checkin | |||
| co: version:checkout | |||
| checkout: version:checkout | |||
| cp: version:checkpoint | |||
| checkpoint: version:checkpoint | |||
There was a problem hiding this comment.
fine with dropping the cp line as its already copy. having checkpoint as alias seems good enough to me.
|
|
||
| before_script: | ||
| - composer self-update | ||
| - composer require "symfony/symfony" "~2.8" --no-update | ||
| - composer install | ||
| - composer require "symfony/symfony" "$SYMFONY_VERSION" --no-update |
There was a problem hiding this comment.
requiring symfony/symfony will hide dependency problems. the proper solution these days would be something with flex as explained in https://github.com/symfony/lts
but this is no problem introduced with this pull request, so we don't have to fix it in this PR.
|
@dbu the problem is that travis would have to run 27 builds - should i enhance the matrix? i think in a further PR you have to fix the travis setup at all as you mentioned the symfony require thing :) |
wachterjohannes
force-pushed
the
symfony-4-support
branch
from
638924c to
96b8301
Compare
|
why 27? that would indeed be too much. but i count 12: php 5.6 lowest, 7.0, 7.1 and 7.2, each with the 3 suites. where does the rest come from? |
|
@dbu thats done :) i thought about mixing also the prefer-lowest and the versions that would lead into much more builds. |
|
thanks a ton! |
|
@dbu thanks for this fast review/merging :) which version will contain this change? and when do you think will you release it? |
|
its released as 1.1.1 |
Replaces #199