Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HeaderCommentFixer - support monolithic files with shebang #6199

Closed
wants to merge 6 commits into from
Closed

HeaderCommentFixer - support monolithic files with shebang #6199

wants to merge 6 commits into from

Conversation

kubawerlos
Copy link
Contributor

Such as dev-tools/composer-alias-update.php and dev-tools/info-extractor.php.

@coveralls
Copy link

coveralls commented Dec 20, 2021

Coverage Status

Coverage increased (+0.001%) to 93.15% when pulling 6af55ed on werlos:HeaderCommentFixer_support_non_monolitic_files into 333f15e on FriendsOfPHP:master.

@keradus
Copy link
Member

keradus commented Dec 29, 2021

@kubawerlos , do I understand right that you don't want to support all non-monolithic files, but support monolithic scripts that are having a shebang?

@kubawerlos
Copy link
Contributor Author

@kubawerlos , do I understand right that you don't want to support all non-monolithic files, but support monolithic scripts that are having a shebang?

@keradus well I might named the PR wrong, I meant to support what is changed in isCandidate here:

1 === $tokens->countTokenKind(T_OPEN_TAG);

and actually it could be exactly what Tokens::isMonolithicPhp is checking, which would mean removing this: $tokens[0]->isGivenKind(T_OPEN_TAG) part - that way with this PR all monolithic files would be supported, inclduding "monolithic scripts that are having a shebang".

@keradus
Copy link
Member

keradus commented Jan 4, 2022

let's change the name of PR then, as we are still not supporting all non-monolithic files (and we shouldn't)

Why not changing the implementation of isMonolithic then, instead of changing isCandidate of single rule?

@SpacePossum
Copy link
Contributor

In theory, a file like the following is not monolithic:

some leading text that is echo'd and not a hash/shebang

<?php   // single PHP open tag
echo 123;

as such keeping the method to detect it stating it is not monolithic seems good to me.
For the header fixer to start fixing files like that is good to me (might be a nice test to add),
even though the intended candidate would be like:

#!/usr/bin/env php
<?php

echo 1;

I think it was HHVM that would tokenize a hashbang to a different type than an T_INLINE_HTML, but PHP doesn't.

Since for us here we only care about this for the header fixer I'm +1 as PHP templates are rare and templates not starting an open tag even more rare.

@kubawerlos kubawerlos changed the title HeaderCommentFixer - support non-monolithic files HeaderCommentFixer - support monolithic files with shebang Jan 7, 2022
@keradus
Copy link
Member

keradus commented Jan 10, 2022

I believe we should:

fix regular files

<?php

echo 1;

fix scripts with shebang:

#!/usr/bin/env php
<?php

echo 1;

NOT fix non-monolithic and non-script files, eg

foo bar
<?PHP
echo "baz"
<?PHP
echo "foo bar"
?>
baz

@@ -118,7 +118,7 @@ public function getDefinition(): FixerDefinitionInterface
*/
public function isCandidate(Tokens $tokens): bool
{
return isset($tokens[0]) && $tokens[0]->isGivenKind(T_OPEN_TAG) && $tokens->isMonolithicPhp();
return 1 === $tokens->countTokenKind(T_OPEN_TAG);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's update isMonolithic instead?
#6199 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's merge this cleanup first: #6229 and then have a discussion how to update isMonolithic properly - I've already pushed the changes, but it seems a bit naive solution: https://github.com/werlos/PHP-CS-Fixer/compare/simplify_Tokens_isMonolithicPhp_tests...werlos:update_Tokens_isMonolithicPhp and I believe deserves separate PR.

Then we will continue this one.

Copy link
Member

@keradus keradus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's force-squash during merging

@kubawerlos kubawerlos added the RTM Ready To Merge label Jan 18, 2022
@SpacePossum
Copy link
Contributor

Thank you @kubawerlos.

@SpacePossum SpacePossum removed the RTM Ready To Merge label Jan 20, 2022
@kubawerlos kubawerlos deleted the HeaderCommentFixer_support_non_monolitic_files branch January 21, 2022 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants