Implement Pathauto Patterns UI for DFP Global Targeting

I'm going to give it a try, and if its just to keep @jacine sort of sane.

Here is some initial work, which just covers the UI so far.

Its not really pathauto but it still uses the condition api.

This is too fast. I cannot handle that.

1. I really like the idea and the execution looks great too.
2. If the listing page could have some of the additional details of the pathauto page that'd be great.

3. +++ b/src/Entity/TargetingInterface.php
   @@ -0,0 +1,44 @@
   +  /**
   +   * @return \Drupal\Core\Condition\ConditionPluginCollection
   +   */
   +  public function getConditionCollection();
   +
   +  public function getConditionType();
   +
   +  public function getConditionConfig();
   +
   +  public function setConditionConfig($instance_id, array $configuration);
   

   Could do with docs.

4. +++ b/src/Form/Targeting.php
   @@ -0,0 +1,238 @@
   +    $form['id'] = [
   +      '#type' => 'machine_name',
   +      '#maxlength' => 128,
   +      '#default_value' => $targeting->id(),
   +      '#description' => $this->t('A unique machine-readable name for this DFP targeting. Only use letters, numbers and underscores. Example: top_banner'),
   +      '#machine_name' => [
   +        'exists' => ['Drupal\dfp\Entity\Tag', 'load'],
   +      ],
   +    ];
   

   This is a targeting thing right - not a Tag? Also I think this is a bit weird to require the user to come up with the machine name and not allow them to get descriptive.

5. +++ b/src/Form/Targeting.php
   @@ -0,0 +1,238 @@
   +  protected function buildConditionInterface(array $form, FormStateInterface $form_state) {
   

   Missing docs.

6. +++ b/src/Form/Targeting.php
   @@ -0,0 +1,238 @@
   +  public static function onConditionTypeChange($form, FormStateInterface $form_state) {
   

   Docs

7. +++ b/src/TargetingCalculator.php
   @@ -0,0 +1,114 @@
   +    $targeting_settings = $this->configFactory->get('dfp.settings')->get('targeting') ?: [];
   

   Should do this at the end - saves a query if not needed.

8. +++ b/src/View/TagView.php
   @@ -85,22 +87,32 @@ class TagView {
   -  public function __construct(TagInterface $tag, ImmutableConfig $global_settings, TokenInterface $token, ModuleHandlerInterface $module_handler) {
   +  public function __construct(TagInterface $tag, ImmutableConfig $global_settings, array $targeting_settings, TokenInterface $token, ModuleHandlerInterface $module_handler) {
   

   I think it is a bit friendlier to anything that is customising this module to add this new param at the end.

Thank you for your review alex!

I think it is a bit friendlier to anything that is customising this module to add this new param at the end.

Thank you

Damnit, I just forgot to upload a patch.

I think the "Global targeting" section that shows here should be moved to the "List Targeting" tab (which we should rename - see below). I think it could either be provided as a "global" targeting set provided by the DFP module by default, or even just printed at the top of this page.

Great idea! I would ask @alexpott here though, whether this belongs into this issue, or whether this cleanup should be done separately.

Working on it.

Note: Actually we have an option for taxonomy terms: Its hidden behind "vocabulary", because well, this is what the condition is about. Its about the vocabularies of terms.

Changes of this patch:

• Add label and description support
• Some tries to fix some form errors, see comment in the patch
• Show token info

see screenshot:

.

Here is a quick fix

This patch now takes into account cacheability metadata.

Note: This requires #2847334: ContextAwarePluginBase should implement CacheableDependencyInterface to really work.

Now targets could be defined in multiple places. They have to be merged to not be overridden on the ads site.

here testbot