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

reuse addHeader broken for PHP #436

Closed
Potherca opened this issue Nov 12, 2021 · 5 comments · Fixed by #543
Closed

reuse addHeader broken for PHP #436

Potherca opened this issue Nov 12, 2021 · 5 comments · Fixed by #543
Labels
bug Something isn't working comment-styles Request for a new comment style, or fixing a bug with these good first issue Good for newcomers

Comments

@Potherca
Copy link

Potherca commented Nov 12, 2021

When calling reuse addheader on PHP files the comment header is placed outside the <?php tag, which is incorrect.

To demonstrate, assume we have an example file with this content:

<?php

// some code here

Then this happens:

Multiline Comment

Command

reuse addheader --license="MIT" ./example.php --multi-line

Expected
<?php

/*
 * SPDX-License-Identifier: MIT
 */

Actual
 /*
  * SPDX-License-Identifier: MIT
  */

 <?php

Single line Comment

Command

reuse addheader --license="MIT" ./example.php

Expected
<?php

// SPDX-License-Identifier: MIT

Actual
// SPDX-License-Identifier: MIT

 <?php

@pfefferle
Copy link

There might be an other case, where the PHP file does not start with an opening PHP tag, like for example template files: https://github.com/pfefferle/wordpress-activitypub/pull/148/files#diff-b3a77875bda59f2c2629f9c70326b1a6388d38c2685c7b34180217dc5bae4c56

@mxmehl mxmehl added bug Something isn't working comment-styles Request for a new comment style, or fixing a bug with these labels May 13, 2022
@Potherca
Copy link
Author

@pfefferle For files that do not start with a <?php opening tag, the current rendering may indeed be incorrect...

The "comment" would not actually be a comment, but appear in the HTML page text. This is because the "comment" would not be commented out by PHP as it is not inside any PHP tags.

I don't know how this should be resolved, as adding a HTML comment might not be what the user wants, but cramming the comment into the first <?php tag in the file also seems a bad choice. (Not even getting into <?= shorttags here...)

However, is this bug were fixed, is would be trivial to resolve for cases where PHP is used as HTML template... One could simply make sure there always is a <?php ?> at the top of each template file to contain the comment...

@carmenbianca carmenbianca added the good first issue Good for newcomers label May 16, 2022
@carmenbianca
Copy link
Member

The solution should be to simply add <?php to the same list that currently contains the #! hashbang.

I can do this thursday if someone else hasn't done it yet.

@pfefferle
Copy link

pfefferle commented May 17, 2022

@carmenbianca that will fix the problem with files, that starts with <?php but not for templates that have inline PHP code.

@Potherca I think I would check if the file starts with <?php otherwise I would add a the <?php ?> including the meta-infos.

@Potherca
Copy link
Author

I think I would check if the file starts with <?php otherwise I would add a the <?php ?> including the meta-infos.

That seems like a fair solution to me 👍

bbenno added a commit to rotaract/wp-roundhousekick that referenced this issue Jan 25, 2025
PHP CodeSniffer complains about
- ERROR: You must use "/**" style comments for a file comment
- ERROR: Empty line not required before block comment

Both are connected to the SPDX information achieve reuse compatibility
in aed280b as the reuse-tool adds the
license info as follows:

  <?php

  // SPDX-FileCopyrightText: YEAR AUTHOR
  //
  // SPDX-License-Identifier: LICENSE

  /**
   * FILE HEADER COMMENT
   *
  ...

Some context regarding this format can be found in
fsfe/reuse-tool#436

Serveral ways have been considered to address CodeSniffer's errors:
a) specify SPDX information in HTML comment about the `<?php`,
b) move the SPDX information to the phpdoc file header comment, and
c) update the rulset in phpcs.xml to match the current style.

A common drawback of a) and b) is that changes to the annotation via
reuse-tool (`reuse annotate`) require manual adjustments: with a) reuse
adds additional annotations in the wrong `// comment` style; with b)
reuse-tool replaces the entire file header comment with its SPDX
comment.
Additionally, with a) the SPDX information is less coupled to the actual
PHP code, its objective.
With b) there is the question to where exactly put the SPDX information,
in the description, hijack one of the common phpdoc tags or put it in a
custom one (e.g., `@reuse`).

Eventually, we decided to go with c) which itself comes with the
drawback of having to disable the rules causing the errors, i.e.
`Squiz.Commenting.FileComment` and `Squiz.Commenting.InlineComment`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working comment-styles Request for a new comment style, or fixing a bug with these good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants