🇺🇸United States @john.oltman

I suggest not having these per bundle.

Agreed, I prefer this as well.

we have to block the module update using hook_requirements

By the time this is released we could have close to 1,000 installs. Even if only 5% use an own permission varying by type, that is 50 sites that we have broken by removing functionality without a replacement. I don't want to do that. I don't have an alternative yet. Let's mull this over further.

The use case makes sense and I am good with this as long as existing installs are not affected. Meaning it is done with a checkbox setting on the per-host-entity Registration Settings form (a new base field), the Registration type, or global Registration settings, and an update hook sets the box to unchecked for existing sites. If you want new sites to default to checked, no problem. If putting in global settings, https://www.drupal.org/project/registration/issues/3490561 🐛 From address in registration setting is overwritten Active provides an example.

This should definitely wait until the related issue #3464736 is Fixed, so as to avoid rework.

Note: RegistrationConstraintViolationListInterface will need a removeByCode method, in addition to the previously mentioned hasCode method.

Cacheability can only accumulate, even when removing a violation, so one object at the violation list level should suffice (Drupal core doesn't even provide a method for reducing cacheability). For example the admin overrides submodule checks the registration type when deciding whether to remove errors, this will change to checking on whether to remove a violation, and the registration type must then be added to the accumulated cacheability metadata, which subsequently will be applied to the registration form. In this way the registration form will rebuild when the registration type changes.

Regarding AccessResult::neutral, yes, as is the case throughout Drupal, entities that are checked, regardless of whether they create an immediate impact, must be added to cacheability. In the admin overrides example, the registration type must be added to the accumulated cacheability regardless of whether a violation is overridden and removed. There is nothing new here and the violation list concept with a single bubbleable metadata works without issue.

It would probably be best if I just post an MR so you can see how this will work. I should have one up before the year runs out.

Setting to needs work while we sort out my MR review comment.

Skipped the update hook tests since new "host" replacement permissions are imminent and want this merged before then. Can always add a test later.

All tests are passing. I want to add one more test, for the update hook, proving that a user with 'administer own' lost the ability to do certain things before the update hook runs, and regains them after the update hook runs. Will add that within the next day or two.

I thought there might be an issue with settings access control but on further review it looks okay. But to be sure I'll add some tests, which are missing, to RegistrationSettingsAccessTest. Currently there are no tests for "manage own". I'll add some to this MR.

This was ready but then I realized that somewhere along the way "manage own $bundle registration' became too powerful and was allowing settings access. As implied in the description for 'manage $bundle registration settings' permission, settings access requires this additional permission when using either 'manage $bundle registration' or 'manage own $bundle registration' permission. I don't want to have yet another update hook in a separate issue, so we'll have to fix that in this issue. I will work on it.

Also I am on board with needing different levels of checking depending on the context - "available" vs "editable" vs "a complete check on save" vs "change host". I'll be including that concept in the next iteration.

Thanks for the feedback, that is really helpful in understanding all you are trying to accomplish. One thing is that the violation interface supports codes and causes, into which we can stuff our IDs and short messages. So that part is already handled. A violation list cannot be indexed by code though (e.g. I suspect you have existing code that looks like if (isset($errors['close'])) {), so I will need to extend to our own class so it supports a hasCode function. Which then allows me to add the metadata, avoiding the extra parameter. So basically taking your advice to not return the entity violation list but probably a RegistrationConstraintViolationListInterface. Let me build this idea out a bit further and get back in a week or so with a modified proposal.

Thanks @jonathan - this is basically ready - just a few minor nits in the tests. The above makes it look like a lot of work but it's basically the same issues that repeat and I wanted to be sure I flagged all the occurrences.

I like the goal, but think we can get there using patterns and interfaces from Drupal core, instead of inventing new constructs. How about this:

Before:

$errors = [];
$enabled = $host_entity->isEnabledForRegistration($spaces, $registration, $errors);
if (!$enabled and !empty($errors) {
  // process errors
  return;
}

After:

$metadata = new BubbleableMetadata();
$violations = $host_entity->isAvailableForRegistration($spaces, $registration, TRUE /* $return_as_object */, $metadata);
if ($violations->count() > 0) {
  // process violations and apply cacheable metadata to render array
  return;
}

This uses the "return as object" pattern from AccessResults, and the BubbleableMetadata pattern from Token::replace.

I like using the violations interface since EntityConstraintViolationListInterface provides the ability to filter on error types etc.

I like the naming "isAvailable" better than "isEnabled" since "is enabled for registration" and "is configured for registration" are close in meaning, and enabled is overloaded through the "enabled" checkbox on the registration settings page. So it isn't entirely clear through the naming what "is enabled" means. Whereas "isAvailable" is pretty clear.

As part of the MR, isEnabled would call isAvailable and populate its error array using the violations interface. And of course this simple type of call is still supported:

if ($host_entity->isAvailableForRegistration()) {
  // do something when registration is available
}

I'll proceed along these lines unless you can poke some holes in it.

I wonder if we can close this and combine it into https://www.drupal.org/project/registration/issues/3479435 ✨ Create additional host permissions Active

john.oltman → created an issue.

Good catch @elimw, and thanks for the MR. Your fix works for me. I ran the "test only" pipeline and it did not fail, indicating that the test you added doesn't adequately expose the issue. I am guessing because the default user in the test is not in a UTC+1 timezone, the new test passes even without your fix. Changing issue to "needs work" while we iron this out.

john.oltman → created an issue.

I think this would be doable by not adding the initial registration to the cart. Then adding it to the cart when the registration is moved off the wait list to pending. The user would then be emailed to complete checkout. I'll look into adding this to the module. Like the implementation described in the README, it may not work for guest checkout.

I removed setting of the reply-to header since it generally should only be set when it is different from the "From:" address.

Thanks for the post and MR, you are correct, the "from address" is not used currently, since the default mail handler in Drupal core uses the site name and email. This is appropriate for most sites since the site name and email will often be best for email deliverability. However, since there is a "from address" in registration settings, I agree there has to be an option to replace the default behavior in Drupal core for sites that really want to use that instead. I'm going to adjust your MR accordingly.

john.oltman → changed the visibility of the branch 3490561-from-address-in-registration-setting-is-overwritten to active.

john.oltman → changed the visibility of the branch 3490561-from-address-in-registration-setting-is-overwritten to hidden.

Updated the MR with a post update hook and accompanying test to ensure the hook works. Back to ready for review.

Here is the "test only" run showing the missing schema errors when the test view (a simple view of users with a COUNT on user ID) is installed into a site without the accompanying schema fixes: https://git.drupalcode.org/issue/drupal-3263208/-/jobs/3498941

The steps work to replicate, but since there is an MR with a "test only" fail, they are less important now. If you really want to see it, import this view into a test site. https://git.drupalcode.org/project/drupal/-/blob/2bd05246e8bba0ff72332b0...

Aggregating with a COUNT on any entity ID (a user ID, in that example) will create a missing schema error. Look at the artifacts from the MR tests. It's right there.

The MR has been updated, moving hooks in the test module from procedural to OOP. Ready for review.

Added MR, as expected the "test only" pipeline failed due to schema errors, otherwise the fix passes.

This is holding up a recipe for Drupal CMS that cannot pass tests until this is fixed, so hoping this can be reviewed ASAP.

If your store is properly configured, a user will not be prompted for payment info (credit card, etc) while completing checkout if they only have a zero priced item in it. In this case checkout simply entails submitting contact info (name, email, maybe billing address) so you have a proper order. If you are looking to exclude wait list items from checkout entirely, that is different, and you should change the title of this issue to replace the words "payment process" with "checkout" so it is more clear what the goal is. I'll have to think on it if that is the case.

New registrations are checked by RegisterAccessCheck - although it only checks the main status switch in the settings, that should generally be accurate, and the form displays a message instead of the fields when it isn't. So going to leave that use case alone.

There are a number of things in play here:

* Refactoring to share more code between access and validation - I like this idea but let's set this aside since https://www.drupal.org/project/registration/issues/3464736 📌 Replace HostEntity::isEnabledForRegistration Active covers it

* Registration is disabled after the form has already been displayed, e.g. capacity is reached while a new registration form is being filled in - yes, bad UX since there will be an error no matter what the user inputs, but generally, you cannot rely on access control to avoid constraint checks for this reason

* Some validation is dependent on submitted values, e.g. in many cases the registrant is unknown until submit - some checks can only occur in validation, for example the "allow multiple registrations for the same user" check

That leaves us with:

* A non-administrator attempts to edit a registration after registration is disabled, and the disabling occurs before the Edit tab is rendered - we can do something about this one; currently "update" permission is checked in access control - the constraint validator performs the same checks but adds an extra one for this use case. I think the validator is correct in doing this extra check, but it could be added to access control as well.

john.oltman → made their first commit to this issue’s fork.

Replaced a duplicate issue with a different one

Ended up going the other direction and adding the permission since it was a shorter path and some sites could benefit, especially those with many registration types.

Good to hear from you again @dgwolf! Great idea on the summary email - can you open a new issue for that. Thanks!

Hi there, I am guessing you have not read the instructions in the README yet. The user must complete an initial checkout to be placed on the wait list. If you still need assistance after reading those instructions please post again.

@smustgrave should the hooks in the test module be converted to the new Hook system as well - I can do that if needed

The plugin from that module is working for me in my test system (nice module btw!) and the way the select list is built hasn't changed. I recommend clearing cache and if that doesn't work to restore the list, review your workflow at /admin/config/workflow/workflows/manage/registration - to be included, a state has to have its "Show on form" box checked, and there has to be a transition to it from the current state of the applications. If that doesn't help I would need a dump of your site to assist further.

I am changing the Component for this issue, since the Workflow submodule is not involved in building that select list (it controls the workflow action buttons when viewing a registration).

Thanks for the post - if you can explain how that select list is built that would help. I assume you have a custom module that provides a views field plugin or something like that.

Please check the issue queue for existing issues before opening new ones. This is a duplicate of https://www.drupal.org/project/registration/issues/3352096 💬 PHP exception when saving a registration type and entity_redirect module is enabled Closed: works as designed

Good catch, all but field tokens were missing. The host entity title will now be available at [registration:entity] and the host entity next level tokens at [registration:entity:*]. Committed to dev branches and will be in the next release in a few weeks.

john.oltman → created an issue.

john.oltman → created an issue.

The confirmation email can now be translated on the Translate Registration Type page.

The fix has been committed and will be in the next release, which should be within a week or two. It would be best if you can wait for that instead of using the Dev branch, as there will be instructions provided for some of the changes that will be in that release. If you use the Dev branch between now and then, make sure you run a database update and export your configuration.

It is supposed to be possible, but the schema is not set properly for translating. Fix on the way soon.

john.oltman → created an issue.

It's kind of an annoying issue, but I think what I'm doing here is right.

Totally agree. Thanks for all the work you are putting in.

Merged, great stuff Jonathan

Fumbled the commit message on 3.3.x but otherwise it's in and there is no going back. Got it right on 3.1.x.

3.1.x is failing tests due to a composer issue - may be something upstream - will look into later

Question about adding a test

This is very close to being merged.

Ah, makes sense, and I do now recall you mentioning this earlier. Will merge shortly.

Was able to reproduce using the listed steps, will work on a solution.

Thanks for creating this issue, I use it frequently to see where things are at.

Committed #3464137 first

This is really close. Left a couple of comments which may result in code changes. Once those comments are addressed we're good. Thanks for adding the deprecation tests.

john.oltman → changed the visibility of the branch 3473817-hide-state-field-3.1 to hidden.

Better idea is to add the show/hide setting to the form field widget, so each form display can be configured differently as needed.

Going to proceed per my previous comment since no push back.

Reopening, I forgot about 'view label' which is a "pseudo-operation" similar to what you are proposing - so there is a precedent. You won't find a button named "View Label" anywhere but they added the operation to solve a use case. We can do the same.

For the name, we may eventually need a separate "manage" operation someday, so let's stick with "administer" here.

I left a comment on the MR - would feel more comfortable if the "administer" operation was handled more explicitly. Also need tests. Thanks Jonathan!

I updated the previous comment to fix the operation names. We do want plural here as a host has N registrations.