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

CommentFileSizeAuthor
#5 3171493-5.patch22.71 KBPooja Ganjage
#2 3171493-2.patch11.85 KBankithashetty
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ankithashetty created an issue. See original summary.

ankithashetty’s picture

Status: Active » Needs review
FileSize
11.85 KB

Used dependency injection in the following patch, kindly review.

There is usage of \Drupal::getContainer() in class 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!

borisson_’s picture

Status: Needs review » Needs work
  1. +++ b/src/Form/FacetForm.php
    @@ -710,8 +722,7 @@ class FacetForm extends EntityForm {
    -    $already_enabled_facets_on_same_source = \Drupal::service('facets.manager')
    -      ->getFacetsByFacetSourceId($facet->getFacetSourceId());
    +    $already_enabled_facets_on_same_source = $this->facetsManager->getFacetsByFacetSourceId($facet->getFacetSourceId());
    

    This now breaks 80 chars rule.

  2. +++ b/tests/facets_query_processor/src/Plugin/Block/DisplayGeneratedLinkBlock.php
    @@ -13,17 +17,62 @@ use Drupal\Core\Link;
    -class DisplayGeneratedLinkBlock extends BlockBase {
    +class DisplayGeneratedLinkBlock extends BlockBase implements ContainerFactoryPluginInterface {
    

    Does the 80 chars rule apply here as well?
    In any case, this looks like it is a BC break?

  3. +++ b/tests/facets_query_processor/src/Plugin/Block/DisplayGeneratedLinkBlock.php
    @@ -13,17 +17,62 @@ use Drupal\Core\Link;
    +  public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition) {
    

    80 cols here as well.

ankithashetty’s picture

Status: Needs work » Needs review

Hi @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!

Pooja Ganjage’s picture

FileSize
22.71 KB

Hi,

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.

borisson_’s picture

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.

ankithashetty’s picture

Hi @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!

borisson_’s picture

Nice work figuring all that out. That seems to be sufficient information that we should not worry about constructor changes. Thank you!

borisson_’s picture

Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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