Detail page cannot be viewed now in projects coming from RandomDataPlugin. The reason is because the code tries to access a public property which was made private in #3296494: Dynamic properties deprecated in PHP8.2.
Enable "Project Browser Devel" and make sure that RandomDataPlugin is enabled too, then visit project browser and click on the detail page for any project within the RandomDataPlugin. You won't see the detail page. You can check the watchdog and you might see related messages to accessing a private property.
Fix it and provide tests.
Also suggested by @tim.plunkett via slack:
I think we can add a getter. but we should have a dedicated issue either way, to capture the bug in test coverage
| Comment | File | Size | Author |
|---|---|---|---|
| #5 | test_only.diff | 1.04 KB | fjgarlin |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #3
fjgarlin commentedComment #4
fjgarlin commentedTest only file too.
Comment #5
fjgarlin commentedNot sure why the test only fails on svelte bundle if there aren't any changes there at all.
Trying again.
--
Edit: same
Svelte compiled files do not match. Running yarn build and commiting the changs should fix this.In any case, this is not needed as we have the test in the full MR.
Comment #6
fjgarlin commentedIn any case, all the MR is ready and it fixes the regression.
It also adds getters, as suggested, and make the setters chainable, which is a frequent pattern. There is a test for the random plugin detail page, which is what was broken.
In order to test:
Comment #7
tim.plunkettI personally think this MR is overkill. The test is great! But changing all the setters to be fluent is unnecessary (and out-of-scope, even if we did want to do it).
Also I don't know that we need getters for everything. We can add more as we see fit, but I'd rather stick to what we need.
On a practical note, for the getters we DO add, we should have return types.
And for getters for Boolean values like "isCompatible", instead of "getIsCompatible" I think we can stick with one verb and have it stay "isCompatible".
If we ever do decide on fluent setters that return
$this, the docblock should read@return $thiswhich IDEs understand to mean "the same object" as opposed to a new instance that happens to be the same interface.Another out-of-scope question: why did we bother with a ProjectInterface? I hope that wasn't my doing, but it seems overkill for what is essentially a domain object / value object. No one else is going to be subclassing or swapping out that class.
Comment #8
fjgarlin commentedNo worries, I assumed that if we implemented one getter, we might want others, I started to play with multi-cursors in the editor and then things got out of hand.
I've now implemented just the one getter that we need, and as mentioned via slack, I also removed the "ProjectInterface" in favor of just "Project". Most of the MR is the docblocks transfer from one to the other and adding types. I'm happy to do this last part in another issue, but since we mentioned it here I thought that it'd be quicker.
Comment #9
tim.plunkettThanks! Much clearer. And I'm fine with the slight scope addition of dropping the interface, because we weren't going to add the new method to it anyway
Comment #10
tim.plunkettThanks! merged
Comment #12
bnjmnm