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

Generic/CharacterBeforePHPOpeningTag: prevent false positive in mixed PHP/HTML files #3704

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jrfnl
Copy link
Contributor

@jrfnl jrfnl commented Nov 2, 2022

When a file would contain a short open echo tag <?= before a full PHP tag <?php, the sniff would throw a "The opening PHP tag must be the first content in the file" error, which IMO is a false positive.

Fixed now.

Includes additional tests demonstrating the issue and safeguarding the fix.

… PHP/HTML files

When a file would contain a short open echo tag `<?=` before a full PHP tag `<?php`, the sniff would throw a "The opening PHP tag must be the first content in the file" error, which IMO is a false positive.

Fixed now.

Includes additional tests demonstrating the issue and safeguarding the fix.
@gsherwood
Copy link
Member

I don't agree this is a false positive, and I think the proposed change is actually a bit confusing for this specific sniff.

If the short echo represents the start of PHP code, then test file 4 is an error due to the HTML before it. If it doesn't, then test file 5 is an error as the PHP tag isn't the first thing in the file.

What you're proposing is probably a logical rule for projects, but it's not what this sniff was contributed to do. It exists to ensure accidental output of HTML before PHP begins, like when a blank line is added before the opening tag accidentally and you get the error about output already having started. Maybe another sniff that enforces specific rules for mixed HTML/PHP files is required.

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.

3 participants