Problem/Motivation

We are still using the \Drupal::XYZ() facades in some of our classes. We should use dependency injection instead.

Steps to reproduce

Check the latest PHPStan pipeline for the 3.0.x branch (https://git.drupalcode.org/project/recurring_events/-/pipelines?page=1&s...) and search for "\Drupal calls should be avoided in classes".

Proposed resolution

Use dependency injection.

Remaining tasks

Affected classes:

  • ConsecutiveRecurringDateWidget
  • EventInstanceDeleteForm
  • EventInstanceRevisionRevertForm
  • EventInstanceRevisionRevertTranslationForm
  • EventSeriesDeleteForm
  • EventSeriesRevisionRevertForm
  • EventSeriesTypeDeleteForm
  • GroupEventInstanceHandler
  • GroupEventSeries
  • MonthlyRecurringDateWidget
  • RecurringEventsFullCalendarProcessor
  • RegistrantAccessControlHandler
  • WeeklyRecurringDateWidget

API changes

Class constructors might change. This should be done in the 3.x branch.

Issue fork recurring_events-3481021

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

pfrenssen created an issue. See original summary.

divyansh.gupta’s picture

divyansh.gupta commented
Assigned: Unassigned » divyansh.gupta

divyansh.gupta opened merge request !123

divyansh.gupta’s picture

divyansh.gupta commented
Assigned: divyansh.gupta » Unassigned
Status: Active » Needs review

@pfrenssen I have made changes in the classes requested and applied dependency injection and removed direct drupal calls and placed construct and create functions where ever needed and updated existing in some of them.
Please review this MR and let me know if there are any changes to be made from my side.

pfrenssen’s picture

pfrenssen
Location Sofia
commented
Status: Needs review » Needs work

Awesome, thanks so much for working on this. I see there is a test failure, can you have a look? It looks like the introduction of the constructor in WeeklyRecurringDateWidget needs to be adapted to match the parent class.

divyansh.gupta’s picture

divyansh.gupta commented
Assigned: Unassigned » divyansh.gupta

ok I will make the changes accordingly and make the test pass.

divyansh.gupta’s picture

divyansh.gupta commented
Assigned: divyansh.gupta » Unassigned
Status: Needs work » Needs review

I removed the test failure which was due to mismatch of create function with its parent class.
Please review it and tell me if any changes are to be made

pfrenssen’s picture

pfrenssen
Location Sofia
commented
Status: Needs review » Needs work

Thanks for the work so far! I did an initial review, I found a few small issues.

navekshavj’s picture

navekshavj commented
Assigned: Unassigned » navekshavj
navekshavj’s picture

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

navekshavj commented
Assigned: Unassigned » navekshavj
Status: Needs review » Needs work

Looked there were some phpcs errors. will update once fixed

navekshavj’s picture

navekshavj commented
Assigned: navekshavj » Unassigned
Status: Needs work » Needs review
kul.pratap’s picture

kul.pratap commented
Status: Needs review » Needs work

There are still some direct Drupal calls visible in the PHPStan pipeline.

divyansh.gupta’s picture

divyansh.gupta commented
Status: Needs work » Needs review

Made the changes requested, Please review.

ankitv18’s picture

ankitv18 commented
Status: Needs review » Needs work

Please fix the phpunit pipeline, also there are many warnings in the phpstan which needs to be taken care see: https://git.drupalcode.org/issue/recurring_events-3481021/-/jobs/3724042...

 Method                                                                     
         Drupal\recurring_events\Form\EventSeriesRevisionRevertForm::__construct()  
         invoked with 2 parameters, 3 required.

plopesc made their first commit to this issue’s fork.

plopesc opened merge request !157

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
Status: Needs work » Needs review

MR replacing \Drupal calls reported by PHPStan (Remaining ones in PHP classes should still be there).

While I was there, fixed the outstanding PHPStan issues, so PHPStan pipeline is green now as well!

  • plopesc committed 43ffe513 on 3.0.x 
    Issue #3481021 by divyansh.gupta, plopesc, navekshavj, pfrenssen,...
plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
Status: Needs review » Fixed

MR merged

Status: Fixed » Closed (fixed)

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