Refactor membership module to better support plugins. Update namespac… #15
Conversation
|
Obviously a lot going on here -- not sure whether the plugin changes have much impact on your work... I'm back to not really needing a plugin for the membership_term module. While I still have a ways to go, a simple entityreference is looking like the way to go, particularly because I'm assuming a one-to-many relationship between Membership and MembershipTerm -- having a plugin on a Membership isn't sufficient here, especially because the MembershipTerms might be of different bundles for a single instance of a Membership. I certainly could define a plugin provider and instance id, and populate a membership with whatever type the "active" membership term uses, but I have my core user stories mostly working without needing that. I spent some time in the past day figuring out just exactly how to implement a plugin, and create a factory for them, and getting them mostly implemented -- but at this point there's no UI to select them, and then I realized that it was working with no plugin specified. Curious -- how do you populate the plugin on the membership, with your membership providers? Do you think there's value in having membership_term go ahead and implement the plugin, and populate this data? (I'm not needing it for my goals here, but if its useful to implement for the broader membership ecosystem, I don't think it would take me very long to get done at this point...) |
src/Form/MembershipForm.php
Outdated
| @@ -19,6 +19,9 @@ public function buildForm(array $form, FormStateInterface $form_state) { | |||
| /* @var $entity \Drupal\membership\Entity\Membership */ | |||
| $form = parent::buildForm($form, $form_state); | |||
|
|
|||
| // Hide revision message and revision fields... | |||
| $form['revision_information']['#access'] = FALSE; | |||
| $form['revision_log_message']['#access'] = FALSE; | |||
There was a problem hiding this comment.
Is there a reason to hide these? I see your logic for adding a default message on save, however I think there's a use case for revision notes by the admin?
There was a problem hiding this comment.
Hi,
There's certainly a use case for admins leaving comments, should probably change this to a permission. It's quite confusing with these fields visible, though, and for some reason it's showing up twice.
For now was just hiding to make this usable.
| $this->getEventDispatcher()->dispatch(MembershipEvents::UPDATED, $event); | ||
| parent::postSave($storage, $update); | ||
|
|
||
| } |
There was a problem hiding this comment.
Do we really need to throw our own event here? State Machine module emits an event on state changes, and core already has hook_entity_insert() and friends, that while not a Symfony event provides similar functionality. Not opposed to an event per se but if it basically duplicates the hook that might not be worth the extra overhead here.
In the same vein I think we can back out the STATE_CHANGE and EXPIRE invocations in preSave() given that state machine does the workflow transition event. That was a late addition in state machine so I'm thinking that code predates those events being thrown for us.
There was a problem hiding this comment.
Ok. Still haven't gotten to the state change events yet -- probably will be in there today or tomorrow -- so I haven't seen how state_machine handles this.
I was trying to avoid hook_entity_insert() and hook_entity_update() and go with a more OO approach, and this looked like the best way...
What's your sense of preference going forward -- symfony events vs hooks? I would prefer events, just because that's a pattern I use on a bunch of projects (and never particularly liked hooks). But I can live with a hook here if they aren't going to be deprecated... I was surprised to not find any decorator or observers related to the Entity Manager to supersede these hooks...
| @@ -15,32 +15,39 @@ | |||
|
|
|||
There was a problem hiding this comment.
This interface belongs in the membership_term submodule namespace.
| @@ -28,6 +28,15 @@ public function setWorkflowId($workflow_id); | |||
| public function getMembershipType(); | |||
|
|
|||
| /** | |||
| * @return string | |||
There was a problem hiding this comment.
Same as above; belongs in membership_term namespace.
src/Entity/MembershipType.php
Outdated
| * | ||
| * @var string | ||
| */ | ||
| protected $processor_plugin_id; |
There was a problem hiding this comment.
What's with the "processor plugin"? I'm not sure what this is?
There was a problem hiding this comment.
This was/is an instance of \Drupal\membership\Plugin\MembershipProviderInterface. I was thinking it would be useful to have some membership types control the plugin for their membership instances.
However, after getting something actually working, I reached the conclusion you were correct, it needs to be on the Membership instance, not on MembershipType. Will remove.
src/Entity/Membership.php
Outdated
| $fields['user_id'] = BaseFieldDefinition::create('entity_reference') | ||
| ->setLabel(t('Authored by')) | ||
| ->setDescription(t('The owner of the Membership entity.')) | ||
| ->setLabel(t('Primary user')) |
There was a problem hiding this comment.
"Primary" implies some precedence of multiple users... how about just "User"?
There was a problem hiding this comment.
Ok, will change.
We do plan to support multiple users per membership, that is a requirement for several of our clients. So this will essentially be the author who can "change own" and add/remove users to a membership if more than one is allowed. We will want the API to support this, without requiring it...
|
I apologize I haven't looked at these yet; been on another project lately, but I do plan on reviewing soon. Thanks for all the help. |
|
This PR now contains a bunch of changes added to support a fully working membership_term system, now being used in production... |
Some changes around Plugin handling, mostly trying to get an actual membership term plugin created...