Problem/Motivation
There is a ton of dependencies being injected into the base plugin, SecurityCheckBase. I would like to have a look if these are needed for the large majority of plugins. Otherwise, it would be better to keep the injected dependencies to a minimum and have individual plugins pull their dependencies from the container in the create() method.
Dependencies in SecurityCheckBase
- \Drupal\Core\Messenger\MessengerTrait (through use) (5)
- \Symfony\Component\DependencyInjection\ContainerInterface $container; the DI container (1)
- \Drupal\Core\State\StateInterface $state (18*)
- \Drupal\security_review\SecurityReview $securityReview; the security review service (5)
- \Drupal\security_review\SecurityReviewData $securitySettings; the security_review.data service (6)
- \Drupal\Core\Entity\EntityTypeManagerInterface $entityTypeManager (4)
- \Drupal\Core\Entity\EntityFieldManagerInterface $entityFieldManager (1)
- \Drupal\Core\Extension\ModuleHandlerInterface $moduleHandler (7)
- \Drupal\Core\Config\ConfigFactoryInterface $configFactory (1)
- \Drupal\Core\Database\Connection $database; connection to the database (3)
- \GuzzleHttp\Client $httpClient (2)
- \Drupal\Core\Extension\ExtensionPathResolver $extensionPathResolver; the extension path resolver (3)
- \Drupal\Core\File\FileSystemInterface $fileSystem (1)
- \Symfony\Component\HttpFoundation\RequestStack $requestStack (1)
*: Is actually used in code in the Base so can not be removed.
Numbers in brackets the number of plugins using the dependency. Total number of plugins is currently 18.
Apart from these the base also includes DependencySerializationTrait and StringTranslationTrait. The former, I'm pretty sure, is needed to be able to put the plugins in the batch. The latter will always be used to create report snippets, so also makes sense to keep in the Base.
Dependencies used in AdminPermissions
Dependencies used in AdminUser
None, although the entity loading occuring could be done through the ETM.
Dependencies used in ErrorReporting
- StringTranslationTrait
- $configFactory
Dependencies used in ExecutablePhp
- $httpClient
- MessengerTrait
- $securitySettings
Dependencies used in FailedLogin
Dependencies used in Fields
- securityReview
- entityTypeManager
- entityFieldManager
- database
- messengerTrait
Dependencies used in FilePermissions
- securityReview
- securitySettings
- moduleHandler
- fileSystem
- container (to get stream_wrapper.assets, make optional dependency..?)
Dependencies used in Headers
- securityReview
- requestStack
- httpClient
Dependencies used in InputFormats
- moduleHandler
- securitySettings
Dependencies used in LastCronRun
Dependencies used in NamePasswords
- entityTypeManager
- extensionPathResolver
- MessengerTrait
Dependencies used in PrivateFiles
None
Dependencies used in QueryErrors
Dependencies used in TemporaryFiles
- securitySettings
- moduleHandler
Dependencies used in TrustedHosts
Dependencies used in UploadExtensions
- moduleHandler
- securityReview
- entityTypeManager
- extensionPathResolver
- MessengerTrait
Dependencies used in VendorDirectory
None.
Dependencies used in ViewsAccess
- moduleHandler
- securityReview
- entityTypeManager
- extensionPathResolver
- MessengerTrait
Proposed resolution
Go over the various plugins and analyse which dependencies they actually use. Keep general dependencies in the base class (t is meant to isolate common code after all) but move less common dependencies out and add them only to the relevant plugins.
I think it makes a certain amount of sense to keep the securityReview and securityReviewData dependencies in the base. Other than that, given the amount of overlap, I actually don't think any of the dependencies should be kept in the base.
Remaining tasks
- Apply changes
- Create merge request
- Merge
User interface changes
None
API changes
SecurityCheckBase will have less ready-to-go dependencies.
Data model changes
None.