Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
$ phpcs --standard=DrupalPractice .
FILE: /facets/src/Controller/FacetBlockAjaxController.php
---------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
---------------------------------------------------------------------------------------------------------
137 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
---------------------------------------------------------------------------------------------------------
FILE: /contrib/facets/src/Form/FacetSourceEditForm.php
----------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------------------------------
180 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
----------------------------------------------------------------------------------------------
FILE: /contrib/facets/src/Form/FacetForm.php
----------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------------------------------
713 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
----------------------------------------------------------------------------------------------
FILE: /contrib/facets/src/Form/FacetSettingsForm.php
----------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 3 WARNINGS AFFECTING 3 LINES
----------------------------------------------------------------------------------------------
292 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
297 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
315 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
----------------------------------------------------------------------------------------------
FILE: /contrib/facets/src/Form/FacetCloneForm.php
----------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
----------------------------------------------------------------------------------------------
74 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
153 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
----------------------------------------------------------------------------------------------
FILE: /facets/tests/facets_query_processor/src/Plugin/Block/DisplayGeneratedLinkBlock.php
-----------------------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
-----------------------------------------------------------------------------------------------------------------------------------------
23 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
24 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
-----------------------------------------------------------------------------------------------------------------------------------------
FILE: /facets/modules/facets_summary/src/Form/FacetsSummarySettingsForm.php
---------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 3 WARNINGS AFFECTING 3 LINES
---------------------------------------------------------------------------------------------------------------------------
197 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
225 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
244 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
---------------------------------------------------------------------------------------------------------------------------
FILE: /facets/modules/facets_summary/src/Form/FacetsSummaryForm.php
-------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
-------------------------------------------------------------------------------------------------------------------
403 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
-------------------------------------------------------------------------------------------------------------------
Proposed resolution
We can use dependency injection instead of direct /Drupal calls.
User interface changes
NONE
API changes
NONE
Data model changes
NONE
Comment | File | Size | Author |
---|---|---|---|
#5 | 3171493-5.patch | 22.71 KB | Pooja Ganjage |
#2 | 3171493-2.patch | 11.85 KB | ankithashetty |
Comments
Comment #2
ankithashettyUsed dependency injection in the following patch, kindly review.
There is usage of
\Drupal::getContainer()
inclass FacetBlockAjaxController
, but didn't find a way to use DI for this use-case. Came across this issue while researching #2061761: Mark Drupal::getContainer() as internal, may be we can leave\Drupal::getContainer()
as it is...Thank you!
Comment #3
borisson_This now breaks 80 chars rule.
Does the 80 chars rule apply here as well?
In any case, this looks like it is a BC break?
80 cols here as well.
Comment #4
ankithashettyHi @borisson_, thank you very much for the review. Before submitting the patch I did make sure to check any Drupal standard errors in the patch by running
phpcs --standard=Drupal
and didn't find any errors as such...And responding to the changes specified in #3, as per the Drupal coding standard document https://www.drupal.org/docs/develop/standards/coding-standards#linelength, the lines containing longer function names, function/class definitions, variable declarations, etc are allowed to exceed 80 characters.
I request you to review the patch again and please let me know if you would like any corrections to be made... Will be changing status back to "Needs review".
Thanks again!
Comment #5
Pooja Ganjage CreditAttribution: Pooja Ganjage at Asentech LLC commentedHi,
I have seen multiple \Drupal call are used in files.
So, I am applying patch for that.
Kindly review the patch once.
Let me know for any recommendations.
Thanks.
Comment #6
borisson_I'm going to ignore #5, it does not seem to apply and is twice as big without an interdiff.
Like I said in #3.2 - Does changing the constructor/required interfaces count as a BC break? I feel like it does without actually providing any real-world benifit.
This "code cleanup" kind of tasks are important, I agree that code should be clean. But adding a possible BC break and headaches for people extending some of our classes for no actual benefit feels "wrong". If the documentation is not clear about this, or it states that there is no BC-promise there we can get this in.
Comment #7
ankithashettyHi @borisson_, thanks for your review... I did some digging regarding the concern you raised in #3.2, well, all I found was the Constructor BC policy is still under discussion #3030640: [policy, documentation] Clarify Constructor BC policy and document best practices for subclassing other classes , #3050720: [Meta] Implement strict typing in existing code. Based on the changes being made all over contib modules, looks like it's not considered under BC as of now...
And with respect to users extending some of your plugin classes without fear of constructor changes, I think this blog explains it well https://www.previousnext.com.au/blog/safely-extending-drupal-8-plugin-classes-without-fear-of-constructor-changes.
With that said, nowhere it's specified that BC do exists or not exists for contructors or I didn't understand from what's been written here in this document https://www.drupal.org/core/d8-bc-policy... Dead end! Sorry...
If you are skeptical about the patch, I sure can revert the changes made to the
class DisplayGeneratedLinkBlock
and upload a new one. Will that be okay?Thanks!
Comment #9
borisson_Nice work figuring all that out. That seems to be sufficient information that we should not worry about constructor changes. Thank you!
Comment #10
borisson_