Skip to content
New issue

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

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

Already on GitHub? # 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.

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants