search

phpcr / phpcr-shell

PHPCR Shell
MIT License
68 stars 19 forks source link

Symfony 4 support #200

Closed wachterjohannes closed 5 years ago

wachterjohannes commented 5 years ago

Replaces #199

wachterjohannes commented 5 years ago

@dbu did you have an idea why the tests fail with symfony 4.0? there is a strange error with multiple cp commands. did see something like this before? https://travis-ci.org/phpcr/phpcr-shell/jobs/430393461#L661

dbu commented 5 years ago

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?

wachterjohannes commented 5 years ago

@dbu thats a good hint :) i will check this

wachterjohannes commented 5 years ago

@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 cp is for copy should i remove the second entry?

dbu commented 5 years ago

the cp alias is duplicated and stands for 2 different things! i have tested it asap in a current installation and cp is for copy should i remove the second entry?

paging @dantleech :-)

wachterjohannes commented 5 years ago

@dbu @dantleech as you can see here https://github.com/phpcr/phpcr-shell/pull/200/files#diff-a31654ea4b9244c0b548a8e9c11d0b15L52 the version:checkpoint command has two aliases and when we remove one the tests seems to be green

dantleech commented 5 years ago

command has two aliases and when we remove one the tests seems to be green

sounds good to me, I guess somehow before the first one had precedence. vcp would be a better alternative (also fine with omitting it)

wachterjohannes commented 5 years ago

@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?

wachterjohannes commented 5 years ago

@dantleech fixed

wachterjohannes commented 5 years ago

@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 :)

dbu commented 5 years ago

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?

wachterjohannes commented 5 years ago

@dbu thats done :) i thought about mixing also the prefer-lowest and the versions that would lead into much more builds.

dbu commented 5 years ago

thanks a ton!

wachterjohannes commented 5 years ago

@dbu thanks for this fast review/merging :) which version will contain this change? and when do you think will you release it?

dbu commented 5 years ago

its released as 1.1.1