Patch #6 worked for me. Raised issue to Major since it will affect any site using the popular Diff module. Without the patch, the render of the second revision in the Diff comparison fails since the same node is rendered twice in a single page view. This makes it look like the second revision deleted all the content.
Makes sense, thanks @mandclu! One concern is existing installs that rely on having the State field displayed even if it can't be changed, especially when an existing registration is being edited by an admin (uses the same RegisterForm as a new registration). I also think when hiding the field on an existing registration, it should set the value using the value of the registration state and not the one state in the states array, otherwise the state could unwittingly change without the admin knowing.
To address existing installs, an idea is add a field to the Workflow settings at /admin/config/workflow/workflows/manage/registration, it would be labelled "Single State Form Handling" with help text "Indicate how the registration state field should be handled when only one state is configured to be shown on the registration form" and possible values "Hide the State field" and "Show the State field", with "hide" being the default. We would then have a hook_update_nn set the value to "show" for existing sites and workflows. This way existing installs experience no change, and new sites hide it - we can mention in the release notes so existing sites can edit the workflows if they want the new way of doing things.
We'll also eventually need some tests.
Would like your thoughts on my ideas above, setting to needs work while we work through this.
Cacheability has been committed. Will give input on the approach later this week.
Thanks @ jonathanshaw! Committed to 3.1.x and 3.3.x branches
This work has been incorporated as part of https://www.drupal.org/project/commerce_registration/releases/3.3.0-alpha1 →
Committed to new branch 3.3.x
Started new branch 3.3.x since 3.1.x will never be able to support Drupal 9 and Commerce 3 at the same time.
Replaced this issue with https://www.drupal.org/project/commerce_registration/issues/3472901 📌 Upgrade to Commerce 3.0.x Active so I can get an issue fork on the correct branch.
john.oltman → created an issue.
This has been released as part of https://www.drupal.org/project/registration/releases/3.3.0 →
There were some errors in the 3.2.x branch (my fault) so Drupal 11 support is now available here:
https://www.drupal.org/project/registration/releases/3.3.0 →
Use 3.3.x branch going forward.
john.oltman → created an issue.
john.oltman → created an issue.
john.oltman → created an issue.
Not sure why this was marked fixed since it was not committed to 3.1.x branch yet. Please reopen this issue or commit to the branch and do another release. Thank you.
Thanks for that analysis which is spot on. I've updated the 'steps to reproduce' to a much narrower use case, which is when the extra field builds a render array containing a block content entity. The layout_builder_entity_view_alter() function calls ExtraFieldBlock::replaceFieldPlaceholder() when it finds an extra field in its layout, and that function does a simple replacement of the content for the plugin. If that content contains a render array generated by a call like \Drupal::entityTypeManager()->getViewBuilder('block_content')->view($block_content_entiy, 'default'), the block content suggestions logic will trigger this issue.
Creating a test that instantiates the above would be doable but non-trivial, and more work than I have time for today. I'll put this back into "needs review" to get your thoughts on whether that complicated test is worth it. My thought is that the suggestions logic should be defensive in its approach, and the existing test covers that, but put this back into "needs work" if you feel the complicated test is a requirement to proceed.
I messed up the commit message but this has been committed to 3.2.x and will be in the next release. Will release 3.2.1 this weekend.
Looks good, will test locally and assuming all good will get this merged - thanks for the work!
The easiest approach is to use any of the node tokens in your email body. For example you can have an email subject such as "Thank you for registering for [node:title]" and the email body can include node field tokens like [node:field_something_or_other]. This assumes your host entities are always nodes, which seems likely based on your post. Another approach is to write a custom module that subscribes to the REGISTRATION_ALTER_MAIL event - it gets passed the host entity (presumably a node for your case), so you could do your own replacements or alterations to the mail body using custom logic in PHP. When the event is fired by the confirmation email it has a mail tag you can check, to be sure you are altering the correct email.
Opened MR
Re-roll for 2.1.1 release
Makes sense, thank you.
Added a test. The test passes with the MR and fails when run using the "test only" pipeline, as desired.
Wasn't ready to release this yet and needed to get 3.1.5 and 3.2.0 out.
If you are using Drupal 10.3 or 11 for your development, you may want to change the target branch to 3.2.x for your various issues in progress. Otherwise carry on with 3.1.x and I can test things on 3.2.x.
john.oltman → changed the visibility of the branch 3468752-test-additional-drupal to active.
john.oltman → changed the visibility of the branch 3468752-test-additional-drupal to hidden.
john.oltman → created an issue.
john.oltman → created an issue.
Added credit - should have 3.2.0 release out within a few days
The patch attached to this comment is a re-roll based on the MR. The original patch in #2 does not pass tests because it reorders the suggestions.
I am good with it, please proceed. When merging please edit the commit message so it reads Issue #3465606: Make compatible with Drupal 11 that helps people reviewing the commit log, thanks!
I opened the following related issue https://www.drupal.org/project/drupal/issues/3468180 🐛 Undefined array key "view_mode" in block_content_theme_suggestions_block_alter Active
The attached patch currently applies to the 11.0.x and 10.3.x and 10.4.x branches.
john.oltman → created an issue.
I'm on board with this generally, want to work with it a bit more locally, but the changes look appropriate. The fact that tests are passing (thanks for test updates) says a lot.
Ok, let's plan on that. I'd like to do a 3.1.5 release within the next few days, then branch 3.2.x off at that point. Then you can re-open your MR against that new branch and we'll do a 3.2.0 release immediately after committing this. If you see any issues with this timing or plan please comment.
Merged to dev and will be in the next release. Thanks @jonathanshaw!
Merged to dev and will be in the next release. Thanks for the work!
I am mulling over whether we should cutover to a 3.2 release for this, since support for D9 is (appropriately) dropped in this MR.
Thanks for the help @mandclu! Added you as maintainer.
Excellent suggestion @jonathanshaw!
I'm on board conceptually. The MR is failing tests, and somewhere you will need to implement the "administer" access such that it translates into checking the various administer permissions - I do not see that logic anywhere - it seems to fall through to a case where it checks the administer permissions, plus permissions that do not exist ("administer any xxx").
I believe this is a duplicate of https://www.drupal.org/project/registration/issues/3444198 💬 AJAX or path issue with views_data_export on manage registrations view Closed: works as designed
Added a commit and tests are now passing. I will try to add a new test this week that proves that the old method triggers a deprecation warning and the new method doesn't. Then I can merge it.
@Axael give the code in my previous comment a try - it only emails when a registration is added, and does not need to check state since you only have one.
<?php
namespace Drupal\my_module\EventSubscriber;
use Drupal\Core\Action\ActionManager;
use Drupal\Core\StringTranslation\StringTranslationTrait;
use Drupal\registration\Event\RegistrationEvent;
use Drupal\registration\Event\RegistrationEvents;
use Drupal\user\EntityOwnerInterface;
use Psr\Log\LoggerInterface;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;
/**
* Provides a registration event subscriber.
*/
class RegistrationEventSubscriber implements EventSubscriberInterface {
use StringTranslationTrait;
/**
* The action plugin manager.
*
* @var \Drupal\Core\Action\ActionManager
*/
protected ActionManager $actionManager;
/**
* The logger.
*
* @var \Psr\Log\LoggerInterface
*/
protected LoggerInterface $logger;
/**
* Constructs a new RegistrationEventSubscriber.
*
* @param \Drupal\Core\Action\ActionManager $action_manager
* The action manager.
* @param \Psr\Log\LoggerInterface $logger
* The logger.
*/
public function __construct(ActionManager $action_manager, LoggerInterface $logger) {
$this->actionManager = $action_manager;
$this->logger = $logger;
}
/**
* Process insert.
*
* @param \Drupal\registration\Event\RegistrationEvent $event
* The registration event.
*/
public function onInsert(RegistrationEvent $event) {
$registration = $event->getRegistration();
// Send a confirmation email to the route author for newly completed registrations.
if ($registration->isComplete()) {
if ($host_entity = $registration->getHostEntity()) {
if ($entity = $host_entity->getEntity()) {
if ($entity instanceof EntityOwnerInterface) {
$configuration['recipient'] = $entity->getOwner()->getEmail();
$configuration['subject'] = $this->t('Someone registered for route @title', [
'@title' => $entity->label(),
]);
$configuration['message'] = [
'value' => $this->t('Example email body for confirmation email to route author'),
'format' => 'plain_text', // change this as needed
];
$configuration['mail_tag'] = 'my_module';
$configuration['log_message'] = FALSE;
$action = $this->actionManager->createInstance('registration_send_email_action');
$action->setConfiguration($configuration);
if ($action->execute($registration)) {
$this->logger->info('Sent route author confirmation email to %recipient', [
'%recipient' => $configuration['recipient'],
]);
}
}
}
}
}
}
/**
* {@inheritdoc}
*/
public static function getSubscribedEvents(): array {
return [
RegistrationEvents::REGISTRATION_INSERT => 'onInsert',
];
}
}@jonathanshaw check your custom modules for places where registrations are saved. It is likely the order paid is trying to resave a registration that was saved elsewhere in the checkout flow within the same page load, and thus the registration attached to the order is an outdated in-memory version. To fix your bug, you can do a service override of the event subscriber in commerce_registration and in your custom onOrderPaid event, reload the registration using loadUnchanged (so it loads from db instead of cache) before setting the complete state and saving. You may need to do this in other places if you have registration saves occurring in different places.
Setting this to "works as designed" since this is related to your custom checkout flow and panes. If there wasn't an existing user base I'd consider the change but no need to upset the applecart since nobody else is reporting this.
@Axael the problem is the sample code I posted assumes the registration is created in one state, and then transitions later to complete. Do you have other states or just complete? Based on your answer I can post a modified version of the sample code.
Thanks @jonathanshaw, good catch and what you have looks right to me. There are many instances of "getHandler('registration', 'host_entity')" in the module beyond wait list - I am thinking we should correct those to use the new pattern "getHandler($entity->getEntityTypeId(), 'registration_host_entity');" rather than using the deprecated example.
Thanks @jonathanshaw! Committed to dev branch and will be in the next release.
john.oltman → made their first commit to this issue’s fork.
There are no patches or merge requests for the 3.x version to review yet. Setting back to "active".
Can you send screenshots of (1) this page: /admin/config/workflow/workflows/manage/registration and (2) the registration being viewed which is supposed to change state. It looks like you have the correct setup but want to double check. Also, does the user viewing the registration have the permissions to change state and edit registrations - try with an admin account and if that works it means it is a permissions issue for the user you were trying before.
Go ahead with the sandbox module, the timeline for the other solution is unclear, and the data change introduced by your module is small (one new boolean).
The order of the parameters to the "dispatch" function is now backwards when using the new interface - they need to be reversed. Setting issue back to "needs work" so the MR can be adjusted.
The "set state" action will not take place unless you setup a valid state transition from "present" to "certificate" in the workflow. This is typically done from this page in Drupal admin:
/admin/config/workflow/workflows/manage/registration
After installing your ECA model, once I setup the transition, the new state was saved. Prior to creating the transition, I experienced the same issue you did, which makes sense since the action checks for the transition.
PS - sorry for the test emails, I forgot to enable email reroute in my test environment
john.oltman → created an issue.
Committed to dev branch and will be in the next release. Thanks for the work!
john.oltman → created an issue.
Is the condition/trigger for the action that sends email the same as this one
Are there any messages in the logs? Is this on a test system - are you able to set breakpoints and see if the action is even called?
What state are you attempting to change to? The action requires that you pass the new state in. Please let me know the existing state and the desired new state that is passed to the action.
That makes sense, thanks. There are tests in CommerceRegistrationCartTest that cover this general area - if you want to, throw a test in there within the next couple of days, otherwise I'll add it this weekend. Either way I'll plan on getting this work committed soon.
@Axael is the registration complete? The code I posted only sends the email for newly completed registration, not all new registrations.
Hi Susana, this is a new issue since the other issue was dealing with sending an email when state changed, and your issue seems to revolve around wanting to change the state yourself from ECA. Can you create a new issue within the issue queue for Registration and I can follow up to that new issue. Thanks!
Will take a look within the next few days. Would like to have a PHPUnit test covering this case, but tests involving cron are non-trivial. There are some in the base Registration module if you decide you want to have a crack at it. Otherwise I am happy to add the test to your MR.
@jonathanshaw I am changing this to Needs Work. I saw an initial flurry of commits, but I am guessing there is a bit more to do. If I am wrong, please put this back to Needs Review and assign back to me. Thank you for all of your efforts.
@bunty, your module is working nicely for me, thank you again.
At this time I am pursuing a different approach to this general problem in https://www.drupal.org/project/registration/issues/3419239 ✨ Establish host variations as an architectural concept Active that would provide a more flexible solution than yours (for example, supporting not only capacity but other registration related attributes as "global"). I will post a note here once the more general purpose solution is available.
@freelock you are correct that settings are not automatically instantiated (saved to the database) for host entities. Doing so would cause many issues including making it more difficult to change the default settings later. However the host entity object already has the getSettings call that you propose for the RegistrationManager, so another function is not needed. Example code assuming a node is the host:
$handler = \Drupal::entityTypeManager()->getHandler('registration', 'host_entity');
$host_entity = $handler->createHostEntity($node);
$settings = $host_entity->getSettings();
Alternatively, if your site is multilingual:
$settings = \Drupal::entityTypeManager()->getStorage('registration_settings')->loadSettingsForHostEntity($host_entity, $langcode);
The following code is not necessary, but if for some reason your use case requires that a settings entity exist in the database prior to the host having a registration or prior to settings being saved through Drupal admin, then you can do this. This is in fact done in the postSave for the Registration entity.
if ($settings->isNew()) {
$settings->save();
}
Your proposed ECA Action still makes sense to me though, it just doesn't need a new API function to be created.
I do not know enough about ECA to comment on the Tokens you proposed, but they seem logical.
I hope this helps, happy to clarify further if you have more questions.
PS - Looks at the tests folder in the Registration module - lots of goodies in there showing how entities can be fetched and manipulated.
Thank you for the sandbox, I will try this out soon. Looks promising from my initial peek!
Maybe we should add as our helper an addHostIfEnabled(PossibleHostEntityInterface $host) method to RegistrationChangeHostPossibleHostsEvent in addition to addHost(PossibleHostEntityInterface $host).
I like it, sounds like a plan. Please proceed (and thank you).
On further thought, I'm not clear on the need here. It's true that if a developer wants to only have hosts configured present in the list then in their event subscriber they need to load their possible host entities and filter by whether they are enabled for registration. But they could (and should) add the registration settings cache tags of any disabled host to the event at that point, ensuring that the possible hosts list is rebuilt if any of the disabled hosts get enabled.
Wouldn't having the subscriber add a cache tag of [registration_settings:list] be all that is needed? Then any settings update triggers a rebuild of the possible hosts list/display. I thought this was your idea actually. Then the subscriber only needs to add the hosts it wants to display to the list.
I'd prefer "Select {host_type_label}" to "Select new {host_type_label}" for that reason, and I think that works well for either workflow.
Sounds good, I'm on board.
It should invalidate the list cache tags for the host entity. If we did this, the possible hosts cacheability would work fine as long as the subscriber added the appropriate list cache tag.
Good idea, but can we use [registration_settings:list] instead of the host entity list. I'm concerned about forcing a rebuild of every node list block on the site when settings for one event are updated. Sites that want to couple the caching for hosts and settings can use the registration_inline_entity_form module to achieve that, or do custom code. I'm afraid to force it for all sites at this point given there are hundreds of existing installs.
Good points Jonathan and not over-engineering - on the View Registration page the name of the host entity bundle is used. We could probably use that same label everywhere I am proposing to use "host". So the tab would read "Change Event" if the host entity type is an Event content type and page 1 title would be "Select new Event" etc. The relevant function is right on the Registration entity with method name "getHostEntityTypeLabel" and it returns the bundle label if the type has bundles, so "Event" in my example. If you can give that a shot in your changes please do, otherwise I'm happy to add this when you are done with your other work.
Recommended changes below.
UX - Right now it is unclear what the state is after selecting a new host and getting to the update page. Has the new host been applied and saved already? Why am I on an update page - did I click the Edit tab by mistake? If I close my browser at this point is the new host set or lost? Recommended changes: Page 1 title is "Select new host", Page 2 title is "Confirm new host and registration data" and page 2 indicates the previous host and new host somehow (and that you are "about to change" to a new host but it hasn't happened yet). I do not think "Delete" should be a link on page 2 and the button text could be "Save Registration and Set New Host" to make clear that unless the button is pressed nothing has changed yet.
Existing host in possible hosts list - The existing host is always present in the displayed list - either hide it on the display side or add "(existing host)" after the title and make it not selectable. Otherwise it is confusing why it is there and people may wonder "do I have two entities with the same title?"
Possible hosts access vs. display - I think we may need to distinguish between possible hosts used for cacheability (in the list so if they become enabled for registration the Change hosts tab shows up) and possible hosts that are visible to the user. Perhaps add a new method "setVisible" on the PossibleHost object, only hosts that are visible are displayed - selectable if enabled for registration and not selectable if not enabled yet. If there are zero visible enabled hosts, the Change host tab is hidden.
Change host entity operation - this does not update like the Change host tab does, when hosts move between enabled and disabled for registration, due to a bug in Drupal core with entity operation cacheability. So if a possible new host becomes disabled and there are no longer any available hosts, clicking on it gives a 403. So I recommend we remove it until the core bug 🐛 Views entity operations lack cacheability support, resulting in incorrect dropbuttons Needs work is fixed in D11. (I realize I am the one who asked for this in the first place, my apologies, did not realize the core bug would affect this.)
Lack of a default event subscriber - I would like there to be a simple subscriber that is there by default, and easily overridden (we'd put instructions in the README), so that a non-developer can at least see the module in action (and in some cases it might be enough). Perhaps something as simple as putting every entity of the same type and bundle as the existing host into the PossibleHosts list (up to some limit perhaps), and making the first 10 that are enabled for registration visible. I agree that most site builders will need to add their own subscriber that overrides it, but I have to think about the non-developer use case as well - of which there are many based on other support requests I get. I don't see a downside to this as it doesn't create more work for the developer use case and it allows people to try the interface out and make a decision on whether they want to put the work into making it perfect for their site with a custom event subscriber.
README - We'll need this eventually once everything else is set.
Thanks Jonathan for all your work and as I have said before, I can do the legwork on anything you are not comfortable with or do not have time for.
Getting closer. More feedback.
I see giving a chance to edit registration info at the same time as changing host to be a UX feature, not a bug... Changing the host is a significant change in the registration, typically one of the biggest changes possible, and I'd imagine it will very often require at least reconsidering answers given to other questions about the registration.
This makes sense and I'm on board with sending people through the update screen all the time, provided there are some minor UX changes per below.
I'm sceptical of making "host" a user facing concept as explained in #21.
I am not on board with this, because the point of the new submodule is to make "host" a user facing concept so site admins can change it, and the admin clicks on a tab saying "Change host" to initiate the process. So "host" is front and center and pretending it isn't user facing is making the UX more difficult than it needs to be. I have some other changes to recommend as well so I'll post all in one new comment.
Thanks for your latest @jonathan I am still testing but wanted to provide some status.
CI errors - I merged the 3.1.x branch back in to resolve
Access bug - I noticed an issue when flipping hosts between enabled for registration and disabled, so pushed up a fix
Admin routes - the change host routes should be admin so users with access to the admin theme have a consistent experience - pushed up a commit for that
I'll get back with more feedback soon. Overall looking good.