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

PHP 8.1 support incomplete #3799

Open
SharkMachine opened this issue Apr 15, 2023 · 15 comments
Open

PHP 8.1 support incomplete #3799

SharkMachine opened this issue Apr 15, 2023 · 15 comments

Comments

@SharkMachine
Copy link

Code sample

<?php

/**
 * @param One&Two $param
 * @return void
 */
function sigh(One&Two $param): void
{
    // Do you really support PHP 8.1
}

To reproduce
Steps to reproduce the behavior:

  1. Create a file called test.php with the code sample above...
  2. Run phpcs test.php ...
 4 | ERROR | [x] Tag value for @param tag indented incorrectly; expected 2 spaces but found 1
   |       |     (Generic.Commenting.DocComment.TagValueIndent)

Expected behavior

No error

Versions (please complete the following information):

  • OS: Ubuntu 22.04
  • PHP: 8.1
  • PHPCS: 3.7.2
  • Standard: You can see it from above
@SharkMachine
Copy link
Author

#3798

@fredden
Copy link
Contributor

fredden commented Apr 15, 2023

If you add a blank* line between lines 4 & 5 in your sample, do you still get an error? I expect not.
Do you get the same error if the type is not a union type? I expect so.

This seems like an alignment issue, not a (lack of) PHP version support.

* Not an completely blank line, as this is in a docblock, so you will need the correct indentation and an asterisk, but nothing else.

@SharkMachine
Copy link
Author

If you add a blank* line between lines 4 & 5 in your sample, do you still get an error? I expect not. Do you get the same error if the type is not a union type? I expect so.

This seems like an alignment issue, not a (lack of) PHP version support.

  • Not an completely blank line, as this is in a docblock, so you will need the correct indentation and an asterisk, but nothing else.

PHPCS doesn't understand the required spacing when using intersection types.

@SharkMachine
Copy link
Author

@fredden Instead of looking at my problem and help me to solve it, you wanted to belittle me.

@fredden
Copy link
Contributor

fredden commented Apr 24, 2023

I have now been able to find time to answer my own questions/suggestions. (I was away from my machines at the time, and I wanted to get you a quick response to your support query.)

If you add a blank* line between lines 4 & 5 in your sample, do you still get an error?

The following code snippet does not show any error for me from Generic.Commenting.DocComment.TagValueIndent. This suggests that the tool (and the sniff in question) handles this code just fine.

<?php

/**
 * @param One&Two $param
 *
 * @return void
 */
function doThing(One&Two $param): void
{
}

Do you get the same error if the type is not a union type?

The following code snippet does get a complaint from Generic.Commenting.DocComment.TagValueIndent on line 4. This suggests that the (union) type is not relevant in this case.

<?php

/**
 * @param bool $param
 * @return void
 */
function doThing(bool $param): void
{
}

Instead of looking at my problem and help me to solve it, you wanted to belittle me.

This is not true. Perhaps I was influenced by the language that you used in the bug report. There was no malice in my intention. I thought I'd provided some suggestions to triage/refine the bug report.

From the information provided, I don't understand what the problem is that you're trying to report. @SharkMachine, please can you provide some more details to help me understand why this is a PHP 8.1/union type support problem? Is there another code sample that demonstrates the issue you're facing?

@mrVrAlex
Copy link

mrVrAlex commented May 3, 2023

Another case (+ PHP 8.2)

<?php

declare(strict_types=1);

namespace Test;

/**
 * Bla bla bla
 */
readonly class Example
{
    public function test() : void
    {
        echo '123';
    }
}

Show

FOUND 2 ERRORS AFFECTING 2 LINES
------------------------------------------------------------------------------------------------------------------------------------
 7 | ERROR | [ ] The file-level docblock must follow the opening PHP tag in the file header
   |       |     (PSR12.Files.FileHeader.IncorrectOrder)
 9 | ERROR | [x] Header blocks must be separated by a single blank line (PSR12.Files.FileHeader.SpacingAfterBlock)
------------------------------------------------------------------------------------------------------------------------------------

If remove readonly OR put final - all will work.

@fredden
Copy link
Contributor

fredden commented May 3, 2023

@mrVrAlex that sounds like #3727 and not what @SharkMachine was trying to highlight. Can you confirm?

jrfnl added a commit to jrfnl/PHP_CodeSniffer that referenced this issue May 3, 2023
As reported in squizlabs#3799 (comment), the `FileHeader` sniff did not take the new PHP 8.2+ `readonly` OO modifier keyword into account.

Fixed now.

Includes test.
@jrfnl
Copy link
Contributor

jrfnl commented May 3, 2023

@mrVrAlex I agree with @fredden, your issue is unrelated to the one being discussed here and belongs with issue #3727.
Either way, I have pulled a fix now in #3816

gsherwood pushed a commit that referenced this issue May 22, 2023
As reported in #3799 (comment), the `FileHeader` sniff did not take the new PHP 8.2+ `readonly` OO modifier keyword into account.

Fixed now.

Includes test.
@vladimir-borduja
Copy link

@jrfnl Apologies, that bother you. But could you please tell us when is the next release that will include these modifications?

PS: PHP8.1 exited in November 2021, my team has been using it for more than 1 year, and we are still avoiding using read-only classes because our pipelines run code sniffer

@Fahani
Copy link

Fahani commented Oct 15, 2023

@jrfnl we are facing the same issue as @vladimir-borduja, any news about the next release?

Thank you!

@jrfnl
Copy link
Contributor

jrfnl commented Oct 15, 2023

@jrfnl we are facing the same issue as @vladimir-borduja, any news about the next release?

@vladimir-borduja @Fahani Unfortunately no, we all (including me), will just have to wait until @gsherwood has some availability again... Also see #3861

@Tahiaji
Copy link

Tahiaji commented Oct 16, 2023

Another case (+ PHP 8.2)

<?php

declare(strict_types=1);

namespace Test;

/**
 * Bla bla bla
 */
readonly class Example
{
    public function test() : void
    {
        echo '123';
    }
}

Show

FOUND 2 ERRORS AFFECTING 2 LINES
------------------------------------------------------------------------------------------------------------------------------------
 7 | ERROR | [ ] The file-level docblock must follow the opening PHP tag in the file header
   |       |     (PSR12.Files.FileHeader.IncorrectOrder)
 9 | ERROR | [x] Header blocks must be separated by a single blank line (PSR12.Files.FileHeader.SpacingAfterBlock)
------------------------------------------------------------------------------------------------------------------------------------

If remove readonly OR put final - all will work.

Another slightly weird way to fix this is to add a docblock between <?php and declare(strict_types=1);

@jrfnl
Copy link
Contributor

jrfnl commented Oct 16, 2023

@Tahiaji That case has already been fixed in master via #3816

@jrfnl
Copy link
Contributor

jrfnl commented Dec 8, 2023

FYI: A fix for the issue reported by @mrVrAlex (PSR12/FileHeader sniff) is included in today's PHP_CodeSniffer 3.8.0 release.

As per #3932, development on PHP_CodeSniffer will continue in the PHPCSStandards/PHP_CodeSniffer repository. If you want to stay informed, you may want to start "watching" that repo (or watching releases from that repo).

@SharkMachine
Copy link
Author

This still being open doesn't look good for https://wiki.php.net/rfc/dnf_types :<

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants