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.

Sign up for GitHub

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

Jump to bottom

[GH 8334] PHP do not display override hint for constructors #8338

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

[GH 8334] PHP do not display override hint for constructors #8338

wants to merge 1 commit into from
+47 −0

Conversation

NReib
Copy link
Contributor

@NReib NReib commented Mar 15, 2025 •
edited by junichi11
Loading

The following leads to a compile error in PHP - so we should not display the Add Override hint for constructors.

<?php
class newPHPClass {

    public function __construct() {

    }
}

class newPHPClass1 extends newPHPClass {
    #[\Override]
    public function __construct() {
        parent::__construct();
    }
}

$anon = new class extends newPHPClass {
    #[\Override]
    public function __construct() {
        parent::__construct();
    }
};

Before:
00_before

After:
00_after

^Add meaningful description above

Click to collapse/expand PR instructions

By opening a pull request you confirm that, unless explicitly stated otherwise, the changes -

  • are all your own work, and you have the right to contribute them.
  • are contributed solely under the terms and conditions of the Apache License 2.0 (see section 5 of the license for more information).

Please make sure (eg. git log) that all commits have a valid name and email address for you in the Author field.

If you're a first time contributor, see the Contributing guidelines for more information.

If you're a committer, please label the PR before pressing "Create pull request" so that the right test jobs can run.

PR approval and merge checklist:

  1. Was this PR correctly labeled, did the right tests run? When did they run?
  2. Is this PR squashed?
  3. Are author name / email address correct? Are co-authors correctly listed? Do the commit messages need updates?
  4. Does the PR title and description still fit after the Nth iteration? Is the description sufficient to appear in the release notes?

If this PR targets the delivery branch: don't merge. (full wiki article)

@mbien mbien added the PHP [ci] enable extra PHP tests (php/php.editor) label Mar 15, 2025
@apache apache locked and limited conversation to collaborators Mar 15, 2025
@apache apache unlocked this conversation Mar 15, 2025
@NReib
Copy link
Contributor Author

NReib commented Mar 15, 2025

I am receiving a mail "PR run failed" regarding this. What is the meaning of this? It seems to be related to github actions, which I am not familiar with.

NetBeans, Attempt #2 / Check Paperwork on Linux/JDK 17
Failed in 12 minutes and 27 seconds

@matthiasblaesing
Copy link
Contributor

@NReib the unittests are not fully stable, so some of them run with retries. You see the fallout of this. If you have commit rights on the repo you can rerun checks from the "Checks" page of the corresponding PR, else you can at least see what failed.

When you look at the checks page right now, it shows, that all tests ran clean (now).

@matthiasblaesing
Copy link
Contributor

@NReib please check if you can add a unittest for this (I bet @junichi11 would/will say the same). Have a look at org.netbeans.modules.php.editor.verification.AddOverrideAttributeHintTest. If you look at the first test (testImplementsInterface_01) you can glimpse the idea:

The unittests is just a call to checkHints. That call takes the name of a PHP file. The infratracture deduces from the test method and the target filename the name of a "golden" file. The idea here is, that the hinting infrastructure is run on the target file and the result is dumped into the golden file. On the first run the dump is checked manually, then checked in and in the future it is used as validation, that there are no regressions.

For the referenced case, look at:

  • php/php.editor/test/unit/data/testfiles/verification/AddOverrideAttributeHint/testImplementsInterface.php
  • php/php.editor/test/unit/data/testfiles/verification/AddOverrideAttributeHint/testImplementsInterface.php.testImplementsInterface_01.hints

You can experiment by removing the .hints file and see what happens when you run the test (it is expected, that the first run is reported as failed).

@NReib NReib force-pushed the B8334 branch 4 times, most recently from 622032d to f8fb770 Compare March 15, 2025 19:58
@matthiasblaesing
Copy link
Contributor

@NReib you'll need to run the test locally, run it twice, on the first run it will fail (generating the hints file), the second run should be clean. Then check the generated hints file, if it looks sane to you. If it does, add and commit it.

@NReib
Copy link
Contributor Author

NReib commented Mar 15, 2025

@matthiasblaesing thank you for the detailed description. Luckily I already knew how to do that from my first pull request here :) Just struggled to include the generated file to the commit...
Do you receive a message everytime I push something to my pull request?

@mbien mbien added this to the NB26 milestone Mar 15, 2025
junichi11
junichi11 reviewed Mar 15, 2025
View reviewed changes
Copy link
Member

@junichi11 junichi11 left a comment

Choose a reason for hiding this comment

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

Please add screenshots before and after. Thanks!

@junichi11
Copy link
Member

junichi11 commented Mar 16, 2025 •
edited
Loading

@NReib Please use 4 spaces for the example of the commit message and remove the PHPDoc if unnecessary.

@junichi11 junichi11 linked an issue Mar 16, 2025 that may be closed by this pull request
[PHP] Override hint should not display for constructor #8334
Open
@NReib
[GH 8334] PHP do not display override hint for constructors
0e27274 
The following leads to a compile error in PHP - so we should not display the Add Override hint for constructors.
```php
<?php
class newPHPClass {

    public function __construct() {

    }
}

class newPHPClass1 extends newPHPClass {
    #[\Override]
    public function __construct() {
        parent::__construct();
    }
}

$anon = new class extends newPHPClass {
    #[\Override]
    public function __construct() {
        parent::__construct();
    }
};

```
@junichi11
Copy link
Member

Please add screenshots to not a comment but the description. Thanks!

junichi11
junichi11 approved these changes Mar 18, 2025
View reviewed changes
Copy link
Member

@junichi11 junichi11 left a comment

Choose a reason for hiding this comment

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

Thank you for your work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@junichi11 junichi11 junichi11 approved these changes

Assignees
No one assigned
Labels
PHP [ci] enable extra PHP tests (php/php.editor)
Projects
None yet
Milestone
NB26
Development

Successfully merging this pull request may close these issues.

[PHP] Override hint should not display for constructor
4 participants
@NReib @matthiasblaesing @junichi11 @mbien