Problem/Motivation

Multiple properties/methods in SchedulerPluginBase and the module provided Scheduler plugins could be automatically derived.

Proposed resolution

Let's simplify the base class and specific plugins.

Issue fork scheduler-3282982

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:

About issue forks

Comments

DieterHolvoet created an issue. See original summary.

DieterHolvoet opened merge request !50

dieterholvoet’s picture

dieterholvoet commented
Title: Simplify SchedulerPluginBase » Simplify the scheduler plugin implementations
Status: Active » Needs review

I added an abstract implementation for the typeFieldName/getTypes/entityFormIds/entityTypeFormIds methods to SchedulerPluginBase. Since the information those methods provide can be automatically derived, we could consider removing these methods from the plugin class.

Merging this would simplify the implementation of scheduler_eck.

dieterholvoet’s picture

dieterholvoet commented

TODO: Filter out plugins with unmet dependencies in SchedulerPluginManager::findDefinitions

jonathan1055’s picture

jonathan1055 commented

Thanks. This looks very interesting.

jonathan1055’s picture

jonathan1055 commented

Couple of quesions:

  1. What is the function findDefinitions() used for? It is not called directly in any Scheduler code. I can see that it is useful to some 3rd-party modules, but we need to explain it. It has both @inheritdoc and a text description, so is not clear. Or maybe this is a standard function? Looks fairly generic. Is this the preferred way to get rid of the code such as
         if (!\Drupal::moduleHandler()->moduleExists('commerce')) {
      return [];
    }

    which is in all of the existing getTypes() functions, but now does not exist.

  2. The 4th parameter $container->get('string_translation') has been removed from new static() in /src/SchedulerPluginBase.php create(). Is this correct to remove? What are the implications?

Thanks. Overall this is good improvement to the plugins and I'd like to get it committed and into the first full 2.0 release.

dieterholvoet’s picture

dieterholvoet commented
  1. findDefinitions is an overridden method from the parent class. This version filters out plugins with missing module depencencies before they are initialized, removing the need for separate checks per plugin.
  2. That fourth parameter was never used, the PluginBase constructor doesn't have a fourth parameter.
dieterholvoet’s picture

dieterholvoet commented

I'm waiting for this issue to be committed and included in a release, so I can commit #3283203: Simplify the scheduler plugin implementation and make a stable release for Scheduler for Entity Construction Kit (ECK).

jonathan1055’s picture

jonathan1055 commented

OK, if I commit this and then release Scheduler 2.0.0-rc3 would that help? There are still a few things to get done before the full 2.0.0 release.

dieterholvoet’s picture

dieterholvoet commented

Sure, it's just so I can add the right version dependency to the module.

jonathan1055’s picture

jonathan1055 commented

In the new typeFieldName() we have

    return $this->entityTypeManager
      ->getDefinition($this->entityType())
      ->getKey('bundle');
  }

But in the new entityFormIdsByType() we cater for no bundle key:

if ($definition->hasKey('bundle'))

What happens in typeFieldName() if the entity does not have a bundle key? Do we need to cover this scenario and fail gracefully? The four existing plugins all have a bundle key, so I can't test this.

dieterholvoet’s picture

dieterholvoet commented

A good example entity type without bundles is user. The question is: does scheduler support this? I don't think so, since the scheduler options are configured per bundle and since there is currently no way to configure it for a whole entity type.

The simplest solution to me sounds like 'don't make scheduler plugins for entity types without bundles', but just to be safe we could filter out any plugins whose entity type doesn't support bundles in SchedulerPluginManager::findDefinitions and document that behaviour. Another option is to add support, but I don't think that would be super useful.

dieterholvoet’s picture

dieterholvoet commented

Here's a plugin for the user entity type in case you want to try it out:

<?php

namespace Drupal\scheduler\Plugin\Scheduler;

use Drupal\Core\Plugin\ContainerFactoryPluginInterface;
use Drupal\scheduler\SchedulerPluginBase;

/**
 * Plugin for User entity type.
 *
 * @package Drupal\Scheduler\Plugin\Scheduler
 *
 * @SchedulerPlugin(
 *  id = "user_scheduler",
 *  label = @Translation("User Scheduler Plugin"),
 *  description = @Translation("Provides support for scheduling user entities"),
 *  entityType = "user",
 *  dependency = "user",
 *  develGenerateForm = "devel_generate_form_user",
 * )
 */
class UserScheduler extends SchedulerPluginBase implements ContainerFactoryPluginInterface {}
jonathan1055’s picture

jonathan1055 commented

Thanks, yes of course the User entity is a good one to test with. I think filtering out non-bundled entity types in SchedulerPluginManager::findDefinitions is a good idea.

We did have a comment on #3225695-11: Support entity types whose entity bundle type does not end with `_type` and entities without created date about entities without a bundle, but as I said on that thread it would be a whole separate task and quite a major change (probably). There is also #3284451: Support for Block (display) entities which might benefit. But for now, lets just filter out the plugin.

dieterholvoet’s picture

dieterholvoet commented

I changed the implementation to filter out plugin definitions for entity types without bundles.

jonathan1055’s picture

jonathan1055 commented

Thanks for doing that. I have removed the bundle checks in the other places, as they should not be needed now.

dieterholvoet’s picture

dieterholvoet commented

Is there anything left to do here?

jonathan1055’s picture

jonathan1055 commented
Status: Needs review » Fixed

If there is anything, it can be done in a follow up issue. Thank you for suggesting this improvement and for doing the bulk of the work. It is much better now, with nothing to do in the plugin definition except specify the arguments.

jonathan1055’s picture

jonathan1055 commented
Assigned: dieterholvoet » Unassigned

I have released Scheduler 2.0.0-rc3

jonathan1055’s picture

jonathan1055 commented

I have just discovered an error, where I should not have removed the check on $definition->hasKey('bundle') in the commit after #15 above. That is needed when generating the 'type' form ids, such as node_type_add_form, as it stands now we get node_type_page_add_form and the hook_form_alter does not get done.

I will put this code back in to fix it. The RC3 release is broken, but I don't think I can delete a release, so I will make RC4.

Then I need to work out why the automated testing did not discover this error ... and then add more test coverage.

  • jonathan1055 committed 385a52f on 2.x 
    Issue #3282982 by jonathan1055: Fix entityFormIdsByType for type forms
jonathan1055’s picture

jonathan1055 commented

There is also another problem (which was there before I made the change). When there are no entity types/bundles created yet, when creating the first one we do not get the form id returned, so the Scheduler options are not added. On subsequent adds, and when editting an entity type it works fine. This is because getTypes() is empty on the first add, so nothing is processed in the foreach loop. It's only a problem for the entity type forms, so I have added a second parameter and then initialised $types according to that setting.

  • jonathan1055 committed 8636857 on 2.x 
    Issue #3282982 by DieterHolvoet, jonathan1055: Cater for adding the...
dieterholvoet’s picture

dieterholvoet commented

Thanks, I created a first release for scheduler_eck.

jonathan1055’s picture

jonathan1055 commented

I need to add a test for the entity bundle form. I thought we had good test coverage so I was surprised when I found the two bugs after all the tests passed.

Added #3285242: Add test coverage for entity type form

Status: Fixed » Closed (fixed)

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