[2.0.0-alpha3] Improved source plugins organization

ready for review

You are a machine. Looks great and brutal fast. :)
Just one thing:
SourcePluginBase::addContextRequirementsValues is not clear for me. Why is the first argument contexts and not only the context which needs the "RequirementsValues". Maybe we create an own Context Class like "Drupal\Core\Plugin\Context\EntityContext"
and Add a static Method $context = RequirementsValuesContext::create('key', ['value1']) and RequirementsValuesContext::attach($context, ['value1'])

Thank you Christian for reviewing.
about the method you mentioned, i just created it to factorize a piece of code that would:
1: create the context when missing and adds the values provided
2: overwrite the context if it already exists and adds the values provided to the already existing values.

that's why i need the whole contexts array,
from the caller point of view, this makes adding values very easy, one line of code. you provide the context array and the values you want to add to the requirements, and it does the job (the method may also allow to update implementation later maybe by hiding the key in the context)

i put it as a static method in that class, but we could have put it elsewhere if we wanted.
i am not so sure to introduce a new context class as you suggested,
probably more elegant in terms of code, but unfortunately it adds even more code (& complexity ?) without really adding a new functionality.

i wanted this MR to removes code at the end, i think this is achieved if you do no count comment lines.

so i would rather keep it like this for now. what do you think?

ok, i addressed your remark,
created specific classed for context definition and context. It is quite nice actually...
I also kept the method in SourcePluginBase to keep the management of requirements simple from consumer point of view.
please review :)

Sorry for the late answer. I was also fine with the solution before. But I think it is better now in sense of more clear. Let's merge!

Hi guys,

I am not sure i understand everything in this ticket, but i want to share than I love it, especially 3 stuffs:

• the way you are presenting and organizing the work together
• the way you extend Drupal\Core\Plugin\Context\Context and Drupal\Core\Plugin\Context\ContextDefinition
• the removal of specific code in ui_patterns_field_formatters & ui_patterns_view and the addition of generic code in ui_patterns

It would be great if every public method is covered by an interface

• By implementing an already existing interface if possible (this is the "dream" target, it worth taking the time of investigating existing interfaces
• Else, by extending or adding a custom interface

For example, RequirementsContextDefinition::fromRequirements(array $requirements = [], ?string $label = NULL) : RequirementsContextDefinition is not covered by
ContextDefinitionInterface

SourcePluginBase::addRequirementsToContext(array $context, array $values): array; is not described by any interface AFAIK.

Also, it would be great to document this, especially by updating the (now outdated) schema: https://app.diagrams.net/#G1DdZDq3CYjnkev1aT_kpyrX7oNkk_2kNv#%7B%22pageI...

Also, it would be great to document this, especially by updating the (now outdated) schema: https://app.diagrams.net/#G1DdZDq3CYjnkev1aT_kpyrX7oNkk_2kNv#%7B%22pageI...

Hi Piere, both functions are factory functions which are usually not covered by an interface.

So for me
1. RequirementsContextDefinition and RequirementsContext is fine without an interface. I like your proposal of an ?additional? factory method forRequirements(array $requirements = [], ?string $label = NULL) or forRequirements(array $requirements = [], ?string $label = NULL) .
2. SourcePluginBase::addRequirementsToContext(array $context, array $values): array; could we maybe move also to the
RequirementsContext class. I "think" factory should belong to the class which is created.
RequirementsContext::attach(RequirementsContext::forRequirements(['field_property']), $contexts).

Short talk to finalize this. Than we I can rebase my stuff today and also provide the MR for the attribute stuff.

But lets merge. It would be easy to refactor that later.

Sure. If you both agree, let's merge. I am just commenting, you are deciding here.

Ok merged.

thanks guys :)

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