This task is a breakaway from the existing Drupal coding standards effort. While technically more of a best practice than a standard, all direct calls to \Drupal should be replaced with dependency injection. That will also require the TBMegaMenuBuilder class be converted into a service. Reference the patch in comment #4 on the coding standards issue, which already contains a majority of the work required to make this happen.

CommentFileSizeAuthor
#9 3183288-9.patch26.87 KBquondam
#5 3183288-5.patch27.62 KBquondam
#2 interdiff.txt4.76 KBtimotej-pl
#2 3183288-2.patch71 KBtimotej-pl

Issue fork tb_megamenu-3183288

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

quondam created an issue. See original summary.

timotej-pl’s picture

timotej-pl commented
Status: Active » Needs review
StatusFileSize
new 3183288-2.patch71 KB
new interdiff.txt4.76 KB

Hi. Looking at the mentioned patch, there is almost everything already done there. Looking through modules code I found one instance where DI could be implemented and that's in Plugin/Derivative/TBMegaMenuBlock.php. I also fixed some Drupal standards in 'tb-megamenu-frontend.js'.
There's still some more calls of \Drupal, but I think that we cannot use DI on them.
I hope if It was okay to work from Patch #4.
I'm adding an interdiff of changes I made and full patch.

quondam’s picture

quondam commented
Status: Needs review » Needs work

Thanks for picking this up @timotej-pl - appreciate your work on the DI efforts. Any chance you could update your patch so that it only contains those changes - and doesn't include any of the coding standards updates? The project maintainers thought that it'd be good to split those two efforts up in order to make the updates easier to review - as well as track and potentially revert commits if we run into any issues. So the idea for this issue was to make it only include the DI changes.

Also, heads-up that 2965871 moved to RTBC yesterday and will most likely get merged tomorrow - so your best bet would be to pull in that commit, apply only the DI changes from your patch above on top of it and then replace the new service call from the latest patch on 2965871 before re-rolling a new patch for this issue.

If you're able to take care of the above I'd be happy to review your updated work and move this issue to RTBC first before updating the coding standards patch.

quondam’s picture

quondam commented
Assigned: Unassigned » quondam

Been working on re-rolling the patch so I'm picking this one back up.

quondam’s picture

quondam commented
Assigned: quondam » Unassigned
Status: Needs work » Needs review
StatusFileSize
new 3183288-5.patch27.62 KB

Re-rolled patch containing only DI updates attached. All module functionality should be regression tested prior to signing off on the included updates as RTBC.

knaffles’s picture

knaffles commented
Parent issue: » #3186616: Security Advisory Coverage

Scott Weston made their first commit to this issue’s fork.

themodularlab’s picture

themodularlab
He/Him
Primary language English
commented
Status: Needs review » Needs work

Patch #5 is currently failing for me when run on the latest D8 (8.9.12) from both the beta and dev branches.

quondam’s picture

quondam commented
Status: Needs work » Needs review
StatusFileSize
new 3183288-9.patch26.87 KB

@themodularlab looks like some code changed recently and/or some prior work that's been flagged as RTBC and shows up as merged isn't appearing in the 8.x-1.x-dev branch. I've re-rolled the patch from #5 and confirmed that it applies cleanly against that branch under 8.9.12.

themodularlab’s picture

themodularlab
He/Him
Primary language English
commented

@quondam,

Looks like the patch is applying now! I will keep testing, make sure there's no regression bugs, etc.

themodularlab’s picture

themodularlab
He/Him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Patch tested against drupal 8.9.12 and 9.1.2. No issues detected or regression bugs found.

themodularlab opened merge request !4

themodularlab closed merge request !4

  • themodularlab committed 0ffb8c6 on 3183288-replace-calls-to 
    Issue #3183288 by quondam, timotej-pl, Scott Weston, themodularlab:...

  • themodularlab committed 0ffb8c6 on 8.x-1.x 
    Issue #3183288 by quondam, timotej-pl, Scott Weston, themodularlab:...

  • themodularlab committed c4c4e7f on 8.x-1.x 
    Revert "Issue #3183288 by quondam, timotej-pl, Scott Weston,...

  • themodularlab committed 0ffb8c6 on 3183288-replace-calls-to 
    Issue #3183288 by quondam, timotej-pl, Scott Weston, themodularlab:...
themodularlab’s picture

themodularlab
He/Him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

This issue has been merged into the 8.x Dev branch. It is scheduled to be included in the next 8.x-1.x release.

knaffles’s picture

knaffles commented

I ran into an issue which appears to be related to these changes. The issue occurs on the latest 8.x-1.x-dev branch, but not in 8.x-1.x. Here are the steps to reproduce:

  1. Spin up a fresh install of 9.1.3 (though it likely occurs in all D8/D9 versions...this is just what I was using)
  2. Install latest 8.x-1.x-dev branch of TB
  3. Create three new pages and add them to the Main Navigation menu
  4. On the menu's edit screen (/admin/structure/menu/manage/main), reorder the menu links and then hit save.
  5. You'll get a 500 error (The website encountered an unexpected error. Please try again later.)

Here's the error in the logs:

Error: Call to undefined method Drupal\tb_megamenu\TBMegaMenuBuilder::logger() in Drupal\tb_megamenu\TBMegaMenuBuilder->getMenus() (line 105 of /var/www/html/web/modules/contrib/tb_megamenu/src/TBMegaMenuBuilder.php)
knaffles’s picture

knaffles commented
Status: Fixed » Needs work
knaffles’s picture

knaffles commented
Status: Needs work » Fixed

Setting this back to fixed and opened a new ticket for the issue mentioned above in comment #19:

https://www.drupal.org/project/tb_megamenu/issues/3194239

  • themodularlab committed 7babe43 on 8.x-1.x 
    Merge branch '8.x-1.x' into 8.x-1.x-dev

* 8.x-1.x:
  Revert "Issue #...
themodularlab’s picture

themodularlab
He/Him
Primary language English
commented
Status: Fixed » Closed (fixed)
themodularlab’s picture

themodularlab
He/Him
Primary language English
commented
themodularlab’s picture

themodularlab
He/Him
Primary language English
commented
themodularlab’s picture

themodularlab
He/Him
Primary language English
commented