Use setters instead of many ContextDefinition constructor arguments

The setters did not go away. This just allows for constructors to be used. I don't see a reason to remove this as it makes it easier for certain things (annotations) to use the class, and it doesn't inhibit the way you've used it elsewhere. Constructor injection has been preferred at almost all stages of Development during this cycle for good reason. I don't see any reason to change that approach for this class.

At the same time I don't see the need for the static create method on this class at all since:

a.) it conflicts with the more common use case for that method name (i.e. injecting services into a class)
b.) It doesn't actually do anything for us that cannot already be done via the constructor or the existing setters.

That being said, I didn't see a reason to go removing it either since this is a paradigm you seem to have adopted for these *Definition classes. I understand your reasoning around it returning the object, but that's a lot of extra code for setting parameters that I exposed in the constructor already. I really don't see any reason to use this. It's not a better dx.

Eclipse

Yes, and the only system that is ever likely to use it is the existing annotation, in which the setters would be markedly more difficult to use than a simple variable pass through like it is now. I did not remove the ability to do the static calls + methods you're used to, please don't remove the constructor arguments that are optional and make my life easier. This should have 0 impact on your normal approach.

Eclipse

The Annotation class has to expose this this way for the actual annotation process. Having the class it instantiates have the same constructor seemed obvious to me. This keeps the use case the same whether using an annotation or instantiating the class manually. As it stands, the code couldn't be simpler in terms of instantiating a new ContextDefinition from the annotation. Your proposal would complicate that unnecessarily, and currently the class doesn't require any of those constructors. I don't see a reason to go changing it. It's nice for Annotations, your normal process still works, and this is deep enough in the guts that 99% of devs will never see it. The 1% who care to use this are going to get what's going on and why with very little in the way of explanation.

Eclipse

@fago: I'm not sure we need a static create() method - you can do your improved DX with the constructor already?

new ContexDefinition('string')
  ->setLabel(t('A string'))
  ->setRequired(FALSE)
  ->setDescription(t('Some description'));

Should work just fine?

I also does not help as for Rules development where we have to do ugly stuff anyway to convert context definitions from configuration arrays to objects:

/**
   * Converts a context definition configuration array into an object.
   *
   * @todo This should be replaced by some convenience method on the
   *   ContextDefinition class in core?
   *
   * @param array $configuration
   *   The configuration properties for populating the context definition
   *   object.
   *
   * @return \Drupal\Core\Plugin\Context\ContextDefinitionInterface[]
   *   A list of context definitions keyed by the context name.
   */
  protected function createContextDefinitions(array $configuration) {
    $context_definitions = [];
    foreach ($configuration as $context_name => $definition_array) {
      $definition_array += [
        'type' => 'any',
        'label' => NULL,
        'required' => TRUE,
        'multiple' => FALSE,
        'description' => NULL,
      ];
      $context_definitions[$context_name] = new ContextDefinition(
        $definition_array['type'], $definition_array['label'],
        $definition_array['required'], $definition_array['multiple'],
        $definition_array['description']);
    }
    return $context_definitions;
  }

So what we actually need in Rules is a method createFromDumbArray() on the ContextDefinition object.

I'm not really excited about the idea of having an array of keys this class expects through the constructor. Having explicit parameters is a lot more clear.

Eclipse

Which is why all except for the type are optional.

https://github.com/fago/rules/pull/246

Tester passing, waiting for merging.

Thanks, merged that into Rules. But for Drupal core I think this is won't fix otherwise, per Eclipse.

FWIW, I don't think Rules should do this either as it makes your ContextDefinitions mutable... can you imagine someone changing that downstream in the middle of a Rules execution? Bad mojo.

Eclipse