Problem/Motivation

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#5 interdiff_2-5.txt7.49 KBheddn
#5 3382718-5.patch11.65 KBheddn
#2 3382718.patch2.25 KBheddn

Issue fork ui_patterns-3382718

Command icon Show commands

Start 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:

About issue forks

Comments

heddn created an issue. See original summary.

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented
Status: Active » Needs review
StatusFileSize
new 3382718.patch2.25 KB
grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Status: Needs review » Needs work

Hi,

@heddn, thanks for the patch.

But it seems not ready for PHP 8.2 regarding tests results. Or is not related?

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented
Related issues: +#3343198: Improve documentation of hook_theme_suggestions_HOOK_alter()

Working on fixes. I found those failures come from #3343198: Improve documentation of hook_theme_suggestions_HOOK_alter().

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented
Status: Needs work » Needs review
StatusFileSize
new 3382718-5.patch11.65 KB
new interdiff_2-5.txt7.49 KB

This probably is a little overboard. But it fixes the failing test and a few other test warnings that came from phpunit. Apparently naming an abstract class ending in Test isn't desirable any more.

I'm also getting some JS test failures now, but I don't know if that is because of flaky js testing or not, so we'll see how the testbot likes things.

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented

Hi,

Thanks for the updated patch.

Here is my review.

  1. +++ b/src/Definition/PatternDefinition.php
@@ -408,7 +408,14 @@ class PatternDefinition extends PluginDefinition implements DerivablePluginDefin
+    $this->definition['theme hook'] =     $altered_hook = str_replace('-', '_', $theme_hook);

    Why not having only `$this->definition['theme hook'] = $altered_hook`? 

  2. +++ b/src/Plugin/Deriver/AbstractPatternsDeriver.php
@@ -34,10 +34,12 @@ abstract class AbstractPatternsDeriver extends DeriverBase implements PatternsDe
+  protected string $basePluginId;

    Should an inheritdoc be added? 

  3. +++ b/tests/src/Unit/Element/PatternPreviewTest.php
@@ -3,15 +3,16 @@
+ * @group legacy

    Why this addition?

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented

1) That's an oversight. It shouldn't be there.
2) I don't think there is a parent variable to inherit doc from. If we need to repeat the variable name in a comment, I can do that.
3) I don't remember. If it isn't needed to get passing tests, we can remove it.

pdureau’s picture

pdureau commented
Assigned: Unassigned » grimreaper
Status: Needs review » Needs work
grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Assigned: grimreaper » Unassigned

Hi,

I have enabled Gitlab CI and fix the problem of templates suggestions in #3417517: Enable Gitlab CI + Update README + Coding standards.

@heddn, Could you please create a MR on the latest codebase. I think with the change from your first patch only.

heddn opened merge request !58

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented
Status: Needs work » Needs review

Tests pass green on php 8.2 in the bot. I couldn't figure out an easy way to run a test-only pass on the code to demonstrate the error w/o any fixes..

grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented

Thanks a lot for the quick response time.

No problem. It fixes a PHPStan error so I guess it is ok.

I will just push a PHPCS fix.

  • Grimreaper committed 04fecfdc on 8.x-1.x authored by heddn 
    Issue #3382718 by heddn, Grimreaper, pdureau: PHP 8.2: Dynamic...
grimreaper’s picture

grimreaper
Primary language French
Location France 🇫🇷
commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.