Contrib.social

Inject various dependencies into DisplayPluginBase

Open on Drupal.org →
Created on 8 June 2013, about 11 years ago
Updated 11 April 2024, 3 months ago

Problem/Motivation

The DisplayPluginBase class uses services but not they are not properly injected so it cannot be tested properly.

Steps to reproduce

N/A

Proposed resolution

Inject these services for better unit testability. Do that in the child classes too

Remaining tasks


Review
Commit

User interface changes

N/A

API changes

Any class extending \Drupal\views\Plugin\views\display\DisplayPluginBase or its children will require the following parameters to their constructor

      \Drupal::service('views.views_data'),
      \Drupal::service('plugin.manager.views.access'),
      \Drupal::service('plugin.manager.views.cache'),
      \Drupal::service('plugin.manager.views.display_extender'),
      \Drupal::service('plugin.manager.views.exposed_form'),
      \Drupal::service('plugin.manager.views.pager'),
      \Drupal::service('plugin.manager.views.row'),
      \Drupal::service('plugin.manager.views.style'),
      \Drupal::service('plugin.manager.views.query'),

Data model changes

N/A

Release notes snippet

N/A

Original report by dawehner

For better unit testability this converts the display base plugin to get it's dependencies injected.

📌 Task
Status

Needs work

Version

11.0 🔥

Component
Views  →

Last updated 1 minute ago

Created by

🇩🇪Germany dawehner

Live updates comments and jobs are added and updated live.
  • VDC

    Related to the Views in Drupal Core initiative.

Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • First commit to issue fork.
  • Merge request !5928Resolve #2015121 "Inject various dependencies" → (Open) created by dimitriskr
  • Pipeline finished with Failed
    6 months ago
    #73833
  • Pipeline finished with Failed
    6 months ago
    #73836
  • Comment 5 months ago →
    🇬🇷Greece dimitriskr

    dimitriskr → changed the visibility of the branch 11.x to hidden.

  • Pipeline finished with Canceled
    5 months ago
    #81602
  • Pipeline finished with Failed
    5 months ago
    #81604
  • Pipeline finished with Failed
    5 months ago
    #81610
  • Comment 5 months ago →
    🇫🇷France andypost
  • Pipeline finished with Failed
    5 months ago
    Total: 175s
    #94369
  • Comment 5 months ago →
    🇬🇷Greece dimitriskr
  • Pipeline finished with Failed
    5 months ago
    #94378
  • Pipeline finished with Failed
    5 months ago
    Total: 526s
    #94383
  • Pipeline finished with Failed
    5 months ago
    #94393
  • Pipeline finished with Success
    4 months ago
    Total: 787s
    #94684
  • Comment 4 months ago →
    🇬🇷Greece dimitriskr

    I've also created a draft CR for this change

  • Pipeline finished with Success
    4 months ago
    Total: 3190s
    #95011
  • Pipeline finished with Success
    4 months ago
    Total: 470s
    #95288
  • Status changed to Needs review 4 months ago
  • Comment 4 months ago →
    🇬🇷Greece dimitriskr

    Finally, ready for review.

    P.S. I've deliberatly put the issue node instead of the draft CR link to the trigger_error(), per a conversation with @berdir at Slack.

  • Status changed to Needs work 4 months ago
  • Comment 4 months ago →
    🇺🇸United States smustgrave

    Left a comment on the MR.

    But think the MR and maybe title should be updated as not super clear why the Rest plugin is needed to be updated.

  • Status changed to Needs review 4 months ago
  • Comment 4 months ago →
    🇬🇷Greece dimitriskr

    RestExport plugin is updated because it extends PathPluginBase, which itself extends DisplayPluginBase

  • Pipeline finished with Success
    4 months ago
    Total: 513s
    #104516
  • Status changed to RTBC 4 months ago
  • Comment 4 months ago →
    🇺🇸United States smustgrave

    Feedback appears to be addressed.

  • Status changed to Needs work 4 months ago
  • Comment 4 months ago →
    🇬🇧United Kingdom catch

    I think we should use constructor property promotion here.

  • Pipeline finished with Success
    3 months ago
    Total: 961s
    #142792
  • Pipeline finished with Success
    3 months ago
    Total: 1171s
    #142833
  • Status changed to Needs review 3 months ago
  • Comment 3 months ago →
    🇬🇷Greece dimitriskr
  • Status changed to RTBC 3 months ago
  • Comment 3 months ago →
    🇺🇸United States smustgrave

    Good call on the constructor promotion.

    For the follow up issue of removing these deprecations would recommend tagging for novice. Would be excellent for new users.

  • Status changed to Needs work 3 months ago
  • Comment 3 months ago →
    🇬🇧United Kingdom alexpott 🇪🇺🌍

    As per @catch's comment on the MR we can remove the constructor docs everywhere and we can use property promotion on all the classes.

  • Status changed to RTBC 3 months ago
  • Comment 3 months ago →
    🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10

    The issue summary has 'Add BC layer' deleted - this is a fairly common extension point.
    We're not obligated to provide a BC layer, but should we do the right thing and try to avoid hard breaking people's things without warning?

  • Status changed to Needs work 3 months ago
  • Comment 3 months ago →
    🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
contrib.social Blog FAQ Discussions
Production build 0.69.0 2024