Problem/Motivation

We have a number of classes that have nullable arguments in their constructors which would cause fatal errors in case these arguments are actually omitted.

Steps to reproduce

An example from TaxonomyIndexTidEventSeriesDepth:

  public function __construct(array $configuration, $plugin_id, $plugin_definition, VocabularyStorageInterface $vocabulary_storage, TermStorageInterface $term_storage, ?AccountInterface $current_user = NULL, ?EntityTypeBundleInfoInterface $entity_type_bundle_info = NULL, EntityFieldManagerInterface $entity_field_manager = NULL, Connection $database) {
    parent::__construct($configuration, $plugin_id, $plugin_definition, $vocabulary_storage, $term_storage, $current_user);
    $this->entityTypeBundleInfo = $entity_type_bundle_info;
    $this->entityFieldManager = $entity_field_manager;
    $this->database = $database;
  }

The following arguments are optional but if we would instantiate the class without these arguments we would get fatal errors:

  1. AccountInterface $current_user = NULL: This is passed on to the parent class which requires this to be an account object and would throw a fatal error if it is NULL.
  2. EntityTypeBundleInfoInterface $entity_type_bundle_info = NULL: is required by internal methods such as ::buildExtraOptionsForm() which would fatal if this is NULL.
  3. EntityFieldManagerInterface $entity_field_manager = NULL: is required by internal methods such as ::buildExtraOptionsForm() which would fatal if this is NULL.

Proposed resolution

Audit all of our constructors and ::create() methods to check for invalid nullable arguments.

Since this causes potential B/C breaks if existing code is extending our classes, we should do this in the 3.x branch.

API changes

Constructor signatures will change.

List of classes to fix

  • RegistrantDeleteForm
  • EventSeriesDeleteForm
  • EventSeriesForm
  • EventInstanceDeleteForm
  • TaxonomyIndexTidEventSeriesDepth
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:

Comments

pfrenssen created an issue. See original summary.

divyansh.gupta’s picture

Assigned: Unassigned » divyansh.gupta

divyansh.gupta’s picture

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

Added custom filter so module will be more robust and less likely to encounter fatal errors due to null arguments.

pfrenssen’s picture

Thanks for starting this! The goal is to audit all our classes, I mentioned TaxonomyIndexTidEventSeriesDepth as an example.

Maybe we can add a list of affected classes to the issue summary so we can tick them off?

divyansh.gupta’s picture

Hello @pfrenssen,
If you provide me a list of affected classes i can work on them and resolve the nullable argument error it may cause if argument is not passed .
Also if you can check my solution so that i can remove them in remaining files also.
I have found this thing in RegistrantDeleteForm, RegistrantForm, AvailabilityCount,RegistrationCount, WaitlistCount, EventInstanceDeleteForm, EventSeriesDeleteForm, EventSeriesForm classes so if you can confirm that error exists only in these classes or you can provide with more classes.

divyansh.gupta’s picture

Assigned: Unassigned » divyansh.gupta
Status: Needs review » Active
pfrenssen’s picture

Issue summary: View changes
pfrenssen’s picture

You're on the right path! The field items (like AvailabilityCount and WaitlistCount) don't need to be changed, they have a good reason to have nullable arguments since they are extending the TypedData class which does the same.

The rest of the list looks complete. I have added it to the issue summary. Thanks for working on this!

divyansh.gupta’s picture

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

Hello @pfrenssen
I have made changes in the requested files so please review them and let me know if there are any changes to be made.

pfrenssen’s picture

pfrenssen’s picture

Status: Needs review » Reviewed & tested by the community

I had some small remarks, but they are already fixed. Looking good, thanks!

pfrenssen’s picture

Status: Reviewed & tested by the community » Fixed

Awesome, thanks so much for working on this!

Status: Fixed » Closed (fixed)

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