CoreSphere / ConsoleBundle

Commandline interface in browser for Symfony2
BSD 3-Clause "New" or "Revised" License
140 stars 54 forks source link

Do not format output if isDecorated is disabled #75

Closed laszlokorte closed 7 years ago

laszlokorte commented 7 years ago

fixes #73

The problem is that the SymfonyStyle class calculates the output string length with all formatting modifiers stripped. But if the string already contained formatting modifiers that we convert to html elements even if setDecorated(false) has been called the string will get longer that. causing the second parameter to str_repeat to be negative.

laszlokorte commented 7 years ago

@TomasVotruba you set up the travis integration, right? Any ideas why the builds are failing?

TomasVotruba commented 7 years ago

@laszlokorte Not sure. But to find out what is wrong, just:

laszlokorte commented 7 years ago
Method `Double\stdClass\P2::render()` is not defined.

Looks like the mocking of the template interface is not working...

laszlokorte commented 7 years ago

Ok I got it it almost fixed (the symfony/templating package is required for the mocks in the test to work). But one error is still left (only for php5.6): The "--level" option does not exist..

I looks like the --level=psr2 option for php-cs-fixer is passed through to symfony which can not handle it? @TomasVotruba Maybe you can take a look when you have some time.

TomasVotruba commented 7 years ago

I'd be in favor of dropping PHP 5.6 and lower, since they're not maintained anymore.

What do you think? I can prepare PR.

laszlokorte commented 7 years ago

On the one hand I am fine with dropping 5.6 but on the other hand symfony itself seems to still support >=5.5.9 so I feel like we should support the same version range as symfony itself.

TomasVotruba commented 7 years ago

Those versions would be still there to use and download. Tagging new versions doesn't delete old ones.

It's matter of development comfort and promoting good practices.

Personally I bumped packages already to PHP 7.1. And people are still using it, I'd say preferring it works on recent PHP version and using it's features.

laszlokorte commented 7 years ago

Yeah I am totally fine with releasing a new php-7.x only version.

But this PR fixes a bug with symfony-3.2 and this fix should also be available for php-5.6 users. I would prefer to get the travis builds succeeding separately from bumping php to 7.x

TomasVotruba commented 7 years ago

Ok, could you send here link to the line with error in Travis?

laszlokorte commented 7 years ago

https://travis-ci.org/CoreSphere/ConsoleBundle/jobs/203796803#L293-L299

TomasVotruba commented 7 years ago

Thank you!

It's related to using php-cs-fixer using through PHAR: https://travis-ci.org/CoreSphere/ConsoleBundle/jobs/203796803#L268

I recommend installing it via composer: https://github.com/Symplify/Symplify/blob/ba539c879f5383be6cceae22d285db13aab0ccb5/composer.json#L54

The thing you mention is change in PHP-CS-Fixer 2.0, when level changed to rules with @ prefix.

Before:

--level=symfony

After:

--rules=@Symfony
laszlokorte commented 7 years ago

@TomasVotruba thanks, I got it working 👍