There are some dependency injection issues in this module.

CommentFileSizeAuthor
#9 reroll_diff_8-9.txt26.45 KBbzoks
#9 2952924-9.patch42.46 KBbzoks
#8 poll_di_2952924-8.patch41.43 KBidebr
#8 interdiff-6-8.txt34.56 KBidebr
#6 poll_di_2952924_7.patch37.12 KBnkoporec
#6 poll_di_interdiff_6_7.txt4.03 KBnkoporec
#5 poll_dependency_injection_2952924_5.patch35.68 KBuseernamee
#2 poll_dependency_injection_2952924_2.patch29.81 KBnkoporec

Comments

nkoporec created an issue. See original summary.

nkoporec’s picture

nkoporec
Primary language English
Location Slovenia
commented
Assigned: nkoporec » Unassigned
Status: Active » Needs review
StatusFileSize
new poll_dependency_injection_2952924_2.patch29.81 KB

Created a patch to fix dependency injection issues.

Status: Needs review » Needs work

The last submitted patch, 2: poll_dependency_injection_2952924_2.patch, failed testing. View results

useernamee’s picture

useernamee commented

Due to March 17 commit to dev this patch needs to be rerolled.

useernamee’s picture

useernamee commented
StatusFileSize
new poll_dependency_injection_2952924_5.patch35.68 KB

I have rerolled the patch and made some minor changes (private -> protected and I found some missed di).

nkoporec’s picture

nkoporec
Primary language English
Location Slovenia
commented
Status: Needs work » Needs review
StatusFileSize
new poll_di_interdiff_6_7.txt4.03 KB
new poll_di_2952924_7.patch37.12 KB

Tested the patch and found issues, add/edit poll form are breaking the site, also poll is not showing.Created new patch which fixes this errors, the problem is within LoggerChannelFactory, some modules can implement their own LoggerChannelFactory and override Core's so it's better to use LoggerChannelFactoryInterface instead.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work

Needs a reroll.

idebr’s picture

idebr commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-6-8.txt34.56 KB
new poll_di_2952924-8.patch41.43 KB
  1. Reroll against 8.x-1.x
  2. Removed some injected classes that were already available on the base class.
  3. Some coding standard fixes in the changed lines.
bzoks’s picture

bzoks commented
StatusFileSize
new 2952924-9.patch42.46 KB
new reroll_diff_8-9.txt26.45 KB

Rerolled against a current version of 8.x-1.x. Adjusted some dependencies.

nace_fr’s picture

nace_fr commented

Hi. I have taken a look at patch from #9. It applied successfully and it seems that all DI is added. I have also ran Poll tests and there were no issues and also tested modules functionality, which also seems to work properly.
I'll still leave the status unchanged, because this patch has a lot of changes and maybe I missed something, so it would be good if anyone else can also takes a look at this.

bramdriesen’s picture

bramdriesen
he/him
Primary language Dutch
Location Belgium 🇧🇪🇪🇺
commented

This is incorporated in #3417969: Adopt GitLab CI template. Also it's better to extend the parent create instead of overriding it. This will prevent fatal errors when the parent would expect a new dependency.

I'll set it to fixed here to give some credit for the work that was done here.

bramdriesen’s picture

bramdriesen
he/him
Primary language Dutch
Location Belgium 🇧🇪🇪🇺
commented
Status: Needs review » Fixed
Parent issue: » #3417969: Adopt GitLab CI template
bramdriesen’s picture

bramdriesen
he/him
Primary language Dutch
Location Belgium 🇧🇪🇪🇺
commented

Status: Fixed » Closed (fixed)

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