Replace HostEntity::isEnabledForRegistration

Created on 30 July 2024, 5 months ago

Problem/Motivation

HostEntity::isEnabledForRegistration() is a great method. The basic idea of defining checks on a host entity and allow customising of those checks makes great sense.

But there are various problems:

1. Event subscribers, or custom host entity handlers, can't add cacheability metadata. So anyone who uses a custom event subscriber that considers anything other the host and its settings could find themselves encountering hard to trace bugs in production.

2. Consuming code has to understand all of the cacheability. Render arrays can make use of HostEntity::addCacheableDependencies, but RegisterAccessCheck for example has to know all about the cacheability involved.

3. There's no method that allows considering isUserRegistered(), isEmailRegistered(), isEnabledForRegistration() altogether. IT wasn't until quite late in developing registration_change_host that I realise that isEnabledForRegistration() was complete indifferent to the allow_multiple setting.

4. The signature of the method includes:
* @param \Drupal\registration\Entity\RegistrationInterface|null $registration
* (optional) If set, an existing registration to exclude from the count.
So it's somewhat explicit that the purpose of passing the registration is only to exclude from the count, it shouldn't be used for any other purpose. But the RegistrationConstraintValidator says:
// Skip certain checks if the host entity is considered enabled for
// registration. This allows event subscribers to override constraint
// checking.
So it explicitly encourages customising the isEnabledForRegistration event, and subscribers may be scrutinising the passed in registration for this purpose, making the expectations around the API here somewhat unclear.
HostEntity::needsCapacityCheck() for example is already considering the state of the registration.

5. The RegistrationConstraintValidator has to reproduce the whole tangle of complex logic involved in isEnabledForRegistration. It's not DRY, and its sufficiently complex it's really hard to know if it handles every edge case correctly, let alone to customise it.

Proposed resolution

1. Deprecate isEnabledForRegistration()

2. Create a RegistrationResult class (extending AccessResult I think, or at least broadly similar). It would allow specifying the result of
a 'check' of some kind, like an individual item in the current $errors array used by isEnabledForRegistration:
- an id (like the current $errors array keys)
- a short message
- a longer message
- cacheable metadata
- a field id associated with the issue
- allowed/neutral/forbidden

3. Create a RegistrationResultSet class. It would allow adding, getting, removing RegistrationResult objects from a set, and have its own isAllowed() method. Additionally, it would implement CacheableDependencyInterface itself and return cache tags etc based on the combined set of RegistrationResult objects.

4. Create 2 new methods:
getRegistrationValidityResult(RegistrationInterface $registration);
This:
- creates a RegistrationResultSet object
- checks isEmailRegistered & isPersonRegistered and the allow_multiple setting
- invokes getRegisterabiltyResult() and merges the resulting set.
- dispatches an event to allow customising the RegistrationResultSet

getRegisterabiltyResultResult($spaces, $existing_spaces);
- creates a RegistrationResultSet object
- checks all the same things that isEnabledForRegistration does
- dispatches an event to allow customising the RegistrationResultSet

I think this works, because when I look at the current uses of isEnabledForRegistration() I find that they either:
- don't have an existing registration object or don't care about it, and don't pass it to isEnabledForRegistration()
OR
- they have an existing registration that does matter, but they should really be considering the allow_multiple setting as well

In theory if there was some reason someone really needed behavior like the existing isEnabledForRegistration($spaces, $registration) for their custom use case, then they could use getRegistrationValidityResult($registration) and modify the result set to get what they wanted.

5. RegistrationConstraintValidator simply calls HostEntity::getRegistrationValidityResult() and iterates over the items, adding violations based on them rather than repeating the logic.

Remaining tasks

User interface changes

API changes

Data model changes

📌 Task
Status

Active

Version

3.1

Component

Registration Core

Created by

🇬🇧United Kingdom jonathanshaw Stroud, UK

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @jonathanshaw
  • 🇬🇧United Kingdom jonathanshaw Stroud, UK
  • 🇺🇸United States john.oltman

    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.

  • 🇬🇧United Kingdom jonathanshaw Stroud, UK

    Thanks for picking this up! It's soft blocking Allow admins to change host entity for existing registration Active so I'm very keen on it.

    One thing I didn't elaborate on in the IS but I do think is worth doing is the "shorter message" and "longer message" thing. Our current $errors often have lengthy verbose messages which are designed to work when shown to the user as validation messages when saving a registration.

    These don't work very well when showing a list of hosts are trying to indicate which are not available for registration. In this case, you want simple 2-3 word explanations that make minimal assumptions. This came up for me in registratation_change_host when showing which hosts were and were not available to change to. But it could also come up in something like commerce_registration when trying to indicate which variations are not registerable. Yes, you can hide the host, but sometimes it's confusing to users to hide options that previously existed and sometimes it's better to "gray them out" and give a reason why they're no longer available. Because there could be multiple reasons, and we know nothing about the context, it's good to keep these very short and generic like "No spaces remaining.", "Already registered.", "Deadline passed.".

    The short message could be the default required message, the longer one optional and shown primarily when saving a registration.

    $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 $form;
    }

    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.

    Using the "return as object" pattern sounds like a great idea.

    I'm not sure we need to pass in the metadata object in this way, rather than get the metadata from the returned object. Unless you were thinking plan to return simply EntityConstraintViolationList objects.

    I agree it's good to have both an "set of items" object and an "item" object.

    I doubt that using simply EntityConstraintViolationList alone for the set would work. Yes, it allows filtering by field. But some of our violations aren't associated with a particular field. Drupal's violations lists don't really expect to be altered using events/hooks, so they have no support for items to have ids or some other identifying codes, and to do has/get/set by id. But this is important to us. (we currently use the $errors array keys for this).

    So even if you use EntityConstraintViolationList as a base, I think you will be extending it. In which case you can make it implement a cacheable interface as well and avoid the need for the seperate cacheable metadata object.

    I like the naming "isAvailable" better than "isEnabled" since "is enabled for registration"

    Agreed.

    Because "allow multiple" can't be checked up front (you don't always know the registrant), the logic to check that will need to stay in the RegistrationConstraintValidator.

    I still believe it's good to distinguish between
    $host_entity->isAvailableForRegistration($spaces, TRUE /* $return_as_object */,);
    and something like
    $host_entity->isRegistrationValidForHost($registration);

    The point of the isRegistrationValidForHost() is that you have a registration - existing or unsaved - and so you know the registrant, and so can call isUserRegistered(), check allow multiple, etc. But it doesn't consider aspects of registration validity that are unrelated to the host, like custom fields, etc., so is simpler than calling $registration-isValid().

    Whereas in isAvailableForRegistration() you don't know anything about the registration, it's more abstract.

    I understand that theres no obvious compelling need to split these 2 apart, but I think it decomposes our complex logic here into 3 neat layers that build on each other:
    - isAvailableForRegistration() considers only the host
    - isRegistrationValidForHost() considers isAvailableForRegistration() and other aspects of the registration in the context of the host
    - $registration->validate() considers isRegistrationValidForHost() and anything else about the registration

    For example, in registration_change_host I only want to show hosts to which a registration can be validly changed. But because changing the host means changing the registration type , required fields could be null on the (temporary, unsaved, hypothetical) registration, and so $registration->validate() will likely have violations. But actually I only care about the host, so I want $host->isRegistrationValidForHost($registration) alone in order to assess whether this host is available to change to.

    I'm excited that you will be working on this! I will make time to swiftly discuss or review anything you like.

  • 🇺🇸United States john.oltman

    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.

  • 🇺🇸United States john.oltman

    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.

  • 🇬🇧United Kingdom jonathanshaw Stroud, UK

    That all sounds good. I think we should have the code be required, but that should be easy enough to accomodate; and consider whether or not to allow duplicate codes.

    Two more things I forgot:

    (1) I think the metadata needs to be on the violation, so if you remove (or modify) a violation you naturally remove or modify the associated violation.

    (2) However, we should also allow for circumstances similar to AccessResult::neutral(), i.e. cases where there is no error but there could have been, so cacheability metadata needs to be added so the result is revisited if the circumstances change so there might then be an error.

    We could have something like an isActive() method on our violation object that defaults to false. Only active violations cause the result sets to be invalid, but non-active violations do have their metadata gathered in. However, that messes with the semantics of violation lists in nasty ways.

    I think better is to either
    (a) abandon violation lists altogether;
    (b) allow them to hold a seperate set of almost-violations in addition to the actual violations; or
    (c) allow for metadata to be added explicitly to the violations list for these rare cases only, so that when you get the metadata from the list you get the metadata explicitly added to the list from possible but not actual violations, combined with the metadata from each actual violation.

  • Assigned to john.oltman
  • 🇺🇸United States john.oltman

    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.

  • 🇺🇸United States john.oltman

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

  • 🇬🇧United Kingdom jonathanshaw Stroud, UK

    Cacheability can only accumulate, even when removing a violation, so one object at the violation list level should suffice

    I agree that (even if it might not be absolutely cache optimal) a single object at the list level should suffice. And the simplciity of that is a compelling virtue.

  • Merge request !82#3464736: Update and add tests → (Merged) created by john.oltman
  • 🇺🇸United States john.oltman

    The open MR is only for tests that need to be fixed and added as preparation for the other branch with most of the work. The existing set of tests is not sufficient to regression test the new work.

    The other branch is mostly done, but needs deprecations and new tests. An MR for that branch will be opened in a few days.

    For now, work on adding regression tests is ongoing in the "prep" branch. The "prep" branch will be merged first, then an MR for the other branch will be opened. This issue will remain in needs work until the second MR is ready for review.

  • 🇺🇸United States john.oltman

    Merged MR with prep tests. Opened new MR with the work. Passing tests, but needs deprecations logic and new tests for the new functions. Not ready for review quite yet.

  • 🇺🇸United States john.oltman

    Ready for review when you get a chance @jonathanshaw. Overall things are way easier to understand and lots of duplicate code removed, so thanks for creating this issue. I did have to add a new validator which is where most of the new code is. I ended up closer architecturally to what you first proposed, with the new result class RegistrationValidationResult, which is working out really nicely.

    To add support for your host change submodule, you would drop a new EligibleForHostChange constraint into the module and call it with code similar to this:

    $validation_result = \Drupal::service('registration.validator')->execute('EligibleForHostChange', [$registration, $host_entity]);
    $validation_result->getCacheableMetadata()->applyTo($host_change_form);
    if ($validation_result->isValid()) {
      // Host is eligible.
    }

    If the host is not eligible you can retrieve the violations and output a short message using the cause. Look at the AvailableForRegistration constraint as a guide, as your constraint and its validator need to extend some new registration classes.

  • 🇬🇧United Kingdom jonathanshaw Stroud, UK

    It looks very classy. I'm impressed you managed to penetrate the arcane world of validation so thoroughly.

    I will update my work on registration_change_host to use it and push that in the next few days. I think this will be a good test of the new approach as the use case has quite a few quirks.

    Once that's done I will leave a review here.

    I will probably suggest you review the change host work before committing this, so we consider the two in tandem.

    I also think it'd be good to hold off on any release for now, as these issues interact a bit with Allow non-admins to edit own registration information Active which has delicate BC concerns linked to the change from 🐛 Access & validation mismatch creating/editing a registration on a disabled host Active which is committed but not released.

    Thanks for your work on this, and for everything we've done in 2024!

  • 🇺🇸United States john.oltman

    Sounds like a good plan to me. Look forward to more great collaboration in 2025!

  • 🇬🇧United Kingdom jonathanshaw Stroud, UK

    I've had a chance to look in detail. Lots to like here, like the use of multiple constraints, the cleanness of isAvailableForRegistration() and the constraint dependencies. There's lots of small points I will need to raise, but 3 bigger issues need discussing first.

    1. Warnings vs errors

    I'm afraid I forgot to express clearly what I think should be a requirement in this API: we should be able to distinguish between soft and hard violations, between gathering constraint violations to get useful warning information, and gathering violations to actually prevent something from happening. In practice, this really ends up meaning between constraint violations and constraint violations that are overriddable by admins. The need for this shows up in 2 places so far:

    (a) RegisterForm::form() shows error messages when the form is built. But as we can/will/should prevent the form from being accessed if the registration is the editor can't save it validly, these can/will/should mostly be seen by admins who can ignore them - but it's good to have them as warnings for these admins. At the moment I'm not sure who would ever see these.

    (b) In registration_change_host (and any other circumstance where one ends up listing hosts) it's good to be able to provide constraint violations as warnings to admins even if they can ignore them. Admins should be able to choose to change a registration to a host that is full, but it's good to have a warning of this on that host in the list.

    The simplest way to implement this is to
    - add methods to RegistrationValidationResult suppressViolationWithCode() and getSuppressedViolations()
    - and then have registration_admin_overrides use suppressViolationWithCode() instead of removeViolationWithCode()
    - in RegisterForm::form() also call getSuppressedViolations() to display them as warning messages.

    2. Existing violations

    There's a strong case that when validating an existing registration, many constraints should not cause validation failure and prevent saving the registration, if the constraint was already violated when the registration was last saved. This means that, for example, if an administrator changes the number of spaces on a registration to exceed the maximum_spaces setting, then other custom fields on the registration can still be edited by the registrant without the registrant needing to reduce the spaces.

    We already recognise this principle for capacity in HostEntity::needsCapacityCheck(). This prevents the most egregious example of a registration becoming invalid simply because of other registrations being created by an administrator. But there are other cases we don't yet properly guard: maximum spaces as above, and also unique user/email. Actually the principle should be that any constraint that depends on something about the registration, should not trigger a violation unless the relevant property of the registration has changed.

    My suggestion is to:
    - load the original unchanged registration in the validator and provide it to the constraints as an $original.
    - add a supressViolationWithCode() or similar method to the RegistrationExecutionContext
    - Each constraint could add violations as they currently do, but after adding the violation it could also check $original and suppress its own violation if it was invalid previously, effectively downgrading it to be a warning.

    3. Granular constraints

    I fear that our validation logic is getting scattered around in a confusing way. It's split into 4 layers that makes it quite hard to reason about.
    i - calling code logic e.g. in RegisterForm::form()
    ii - HostEntity methods
    iii - nested conditionals checking 'status', admin permissions and isNew() in the constraints
    iv - the simple checking of settings and adding violations in the constraints

    I think we might end up with a much more understandable system if we used simple granular constraints:
    HostIsEnabled
    HostHasRoom
    HostAfterClose
    HostBeforeOpen
    HostHasSettings
    RegistrationHasHost
    RegistrationExceedsCapacity
    RegistrationUniqueRegistrant

    More responsibility would then be given to the code that assembles the correct constraints into a pipeline that suits a particular situation. That's where the checks on admin permissions and isNew() etc would be.

    Additionally, if calling code wants to not display violation messages for capacity if the host is disabled, let the calling code handle that easily enough; we don't need to complicate the constraints themselves with a conditional check on the 'status' setting.

  • 🇺🇸United States john.oltman

    Thanks for digging into this so quickly. My thoughts:

    1. Warnings vs errors - handle warnings in your custom app. Or if you want them in the change host module, you could display the usage and output something special if the host is full but still eligible. Admin overrides is for a special (and rare) use case and I don't want it dictating an increase in complexity throughout registration core. Also the UX for warnings is really tough - you probably need different messages vs. the errors, and perhaps a heading "The following errors were ignored due to an active administrative override". In the end you'll probably find no amount of warnings can beat a good training doc. Let's keep our interfaces clean, and drive complexity into user land when it makes sense to do so. If Symfony ever adds support for different violation levels (ala setLevel and getLevel) we can revisit.

    2. Existing violations - unique registrant can't be overridden, but the settings could change, so yeah, it's an issue. You're right about max spaces too. Per above, I'm not interested in different flavors of violations at this time, but if the value is an entity, having the original available in the context makes sense, and there is probably a performance benefit too, loading it once rather than making each constraint load it. I'll get this in as soon as all the issues are settled, should be straightforward. Each constraint will need to have its own logic though - for capacity it checks a couple of fields, for max spaces it checks one field, for unique registrant it checks two fields, etc. I don't think we can generalize this in the way you seemed to suggest "any constraint that depends on something about the registration, should not trigger a violation unless the relevant property of the registration has changed". It is up to each constraint to use the original in the way it sees fit.

    3. Granular constraints - Register form does not know about constraints. So I only see 2 layers, the host entity class and the constraints. Doesn't seem that complicated. We definitely do not want to "bubble complexity up", rather we want to drive it down as far as possible, and the constraints are a good place to hide it. The constraints in Drupal core, e.g. the user module, which is most relevant since users register for accounts, look at permissions and whether an account is new. We should continue to follow that example. Also, I may be wrong, but suspect you are trying to get admin overrides support "for free" in your change host module, by having the overrides change fundamental properties about the host, not just the availability. This could lead to the constraints having different results than the host entity methods like isBeforeOpen. This leads to questions - should the methods call the constraints? What does the Registration Status block display - the standard result, or as overridden by admin overrides? Continuing the theme from above, the overrides are a workaround, not fundamental, so I'm not inclined to have them dictate complexity elsewhere. I agree though that the existing check for enabled is backwards - we should be checking open and close, and only if those are valid should we give a message about disabled - which is the opposite of the current implementation. I'll fix that in a follow up MR, so the existing tests continue to function as regression tests on the current MR. Otherwise I am not on board with having complexity move up the food chain, out of the constraints, and into the host entity class. If you want admin overrides support for change host, we can discuss how best to get there in that issue.

    Let's continue the discussion, and I'm quite interested in the "small" issues you have found as well. Thanks for your efforts!

  • 🇬🇧United Kingdom jonathanshaw Stroud, UK

    1. Warnings vs errors - handle warnings in your custom app. ... Admin overrides is for a special (and rare) use case and I don't want it dictating an increase in complexity throughout registration core.

    That makes some sense to me, I might suggest a tweak that makes it easier to handle this kind of use, but that's settled for now.

    2. Existing violations ... having the original available in the context makes sense, and there is probably a performance benefit too, loading it once ... I don't think we can generalize this in the way you seemed to suggest ... It is up to each constraint to use the original in the way it sees fit.

    I agree with you. I had considered a generalized solution, but come to the same conclusion that it was likely impossible and better to leave each constraint to handle it.

    3. Granular constraints - Register form does not know about constraints. So I only see 2 layers, the host entity class and the constraints.

    RegisterForm makes a choice about calling HostEntity::isAvailableForRegistration() or HostEntity::isRegistrationEditable() depending on whether the registration isNew().

    Doesn't seem that complicated. We definitely do not want to "bubble complexity up", rather we want to drive it down as far as possible, and the constraints are a good place to hide it.

    I can honestly report that I find the current approach bewildering. I'm very very familiar with this module now, far more than other developers will be. But even I find my mind becoming paralysed with uncertainty when I try to think about how some of this works and how the different checks interact.

    Take RegistrationConstraint for example. I can look at it and see

          $validation_result = $this->validator->execute([
            'HostEntity',
            'AvailableForRegistration',
            'EditableRegistration',
            'MaximumSpaces',
            'RegistrationCapacity',
            'UniqueRegistrant',
          ], $registration);

    But what does this mean? There's 6 classes I have to inspect to understand, and all have conditional logic. (and I mean this literally, even after having spent 3 hours studying this system in the last few days, I still have to check right now as I can't remember the details)

    It turns out that
    - many - but not all - have this odd dependency on 'status'
    - UniqueRegistrant does more or less what I'd expect, once I understand it's linked to the 'allow_multiple' setting
    - RegistrationCapacity is also pretty straightforward, although why it needs to check the canceled state when there's logic in getActiveSpacesReserved(), I'm not sure.
    - MaximumSpaces seems simple enough, although the host-or-registration thing makes me squint
    - EditableRegistration checks status and close, but only for existing registrations, and for non-admins
    - AvailableForRegistration is only for new registrations, and checks capacity (at least 1), before and open

    This could be made much easier to grok.

    In RegistrationConstraint:

    $pipeline = [
      'RegistrationHasHost',
      'HostHasSettings',
    ];
    if ($registration->isNew()) {
            'HostIsEnabled'
            'HostIsBeforeOpen',
            'HostIsAfterClose',  
    }
    else {
      if (!$admin) {
        'HostIsEnabled',
        'HostIsAfterClose',
      }
    }
    $pipeline += [
            'RegistrationIsUniqueRegistrant',
            'RegistrationWithinCapacity',
            'RegistrationWithinMaximumSpaces',
    ];
    $validation_result = $this->validator->execute($pipeline, $registration);

    And similarly in HostEntity:

      public function isAvailableForRegistration(bool $return_as_object = FALSE): bool|RegistrationValidationResultInterface {
        $validation_result = $this->validator()->execute([
          'HostHasSettings',
          'HostIsEnabled',
          'HostHasSpace',
          'HostIsBeforeOpen',
          'HostIsAfterClose', 
        ], $this);

    The way we're using constraints is actually very different to how core uses them. In core, the calling code is agnostic about what the constraints, it simple defines a situation for validation and lets modules register a constraint for that situation. But in our case, we're providing a lot of the constraints ourselves and letting other modules adjust them later.

    In this way of thinking, the purpose of constraints is NOT to encapsulate large amounts of logic within a single class. Instead, it's to provide a system for assembling small packets of logic into a set that other modules can override. The constraint codes are an important part of our system, and it might well make sense for the codes to not usually be reused between constraints.

    I'm curious what you reckon :)

  • 🇺🇸United States john.oltman

    I see the rationale, let's explore. First, some names would need to change for consistency, e.g. 'HostHasSpace', 'HostIsBeforeOpen' and 'HostIsAfterClose' would need to become something like 'HostHasRoom', 'HostIsAfterOpen' and 'HostIsBeforeClose'. Maybe there would be a constraint named "HostIsOpen" that encapsulates checking both after open and before close. I may want 'IsEnabled' and 'BeforeClose' to contain the logic for admin and new, but set that aside. Let's assume we got to a list of more granular constraints that we both agreed on in terms of naming and the types of logic. What does this mean for validation event subscribers? It becomes hard for them to know when to activate. So I think we would need to name the pipelines, and make that the first parameter to the validator, as in:

    $validator->execute('AvailableForRegistration', [constraint list for availability check], $value);

    The subscribers, like the one in admin overrides, could then key on the pipeline ID. By limiting their scope to a given (small) set of pipelines, we conceptually avoid changing the meaning of "isBeforeOpen" throughout the system.

    Maybe this could work. Let me know what you think. I'll continue to mull over the implications.

  • 🇬🇧United Kingdom jonathanshaw Stroud, UK

    some names would need to change for consistency, e.g. 'HostHasSpace', 'HostIsBeforeOpen' and 'HostIsAfterClose' would need to become something like 'HostHasRoom', 'HostIsAfterOpen' and 'HostIsBeforeClose'.

    Yes, nice.

    What does this mean for validation event subscribers? It becomes hard for them to know when to activate. So I think we would need to name the pipelines, and make that the first parameter to the validator,

    Yes, I was just coming to say exactly that! I think we need this more or less regardless of granularity.

    should the methods call the constraints

    You mused in #23 about whether the host entity methods should call the constraints.

    I believe the answer is no, the constraints should call the host entity methods. The point of the host entity is that they are rovided by an entity handler, so they can vary depending on the idiosyncracies of an entity type; they allow different entity types to use different information sources when answering a particular question.

    The point of the validation pipelines as I see it is to assemble those questions, and the answers provided by the host entity handler, into a neat set with (a) user friendly messages and (b) machine names allowing event subscribers to identify and modify them.

    The calling code specifies the 'questions' it wants answered by assembling a validation pipeline. In the case of HostEntity::isAvailableForRegistration() there's a convenience method on the host entity because this particular set of questions gets asked a lot.

    I'm dubious about HostEntity::isEditableRegistration(). It's part of the access control system. I agree we should use the validation mechanism, in order to minimise circumstances where users can access something that will not validate. But I'm not sure we need to have this as part of the public API of HostEntity.

    Considering this last case makes m rethink me a little on the matter of granularity. Granularity makes our pipelines much more transparent, but it also opens up the possibility that pipelines when displaying a link or granting access to a form can more easily get out of sync with the validation done subsequently at saving (something you avoid by reusing the isAvailable and isEditable constraints). Perhaps the answer is that we shouldn't be revalidating questions about the host entity when the registration is saved. We don't let users change the host when editing a registration, so 'HostIsEnabled', 'HostIsBeforeOpen', 'HostIsAfterClose' etc are questions we should only ask when considering access to edit/create a registration, we don't need to repeat them when saving the registration, we can trust the access control considers this. And then we wouldn't have to worry about access control constraints and validation-at-saving constraints getting out of sync and users accessing forms they couldn't save.

  • 🇺🇸United States john.oltman

    Added commits. Current status of the "big" issues:

    1. Warnings vs errors - we agreed no action at this time
    2. Existing violations - added original to context, updated unique constraint to use it; max spaces fix will come later
    3. Granular constraints - refactored to granular constraints, with new annotation to segment out our pipeline constraints

    I'm dubious about HostEntity::isEditableRegistration(). I'm not sure we need to have this as part of the public API of HostEntity.

    It's needed by the RegistrationAccessControlHandler. Let's set aside for now and see how things develop in that other issue.

    Perhaps the answer is that we shouldn't be revalidating questions about the host entity when the registration is saved.

    We have to revalidate because many things can happen after a form is loaded and by the time the Register or Save button is pressed. For example, the settings could have changed or someone else registered for the last slot. Even the mere passage of time can mean registration is no longer allowed. For an existing registration where the registrant, spaces and status have not changed, the re-validation should result in no violations (this will be true once the spaces fix is in), so all is good.

    Let me know on the smaller issues and we can go from there.

  • 🇬🇧United Kingdom jonathanshaw Stroud, UK

    I've implemented this in registration_change_host, will push an update to that MR today so you can see what it looks like. All pretty straightforward, nothing fancy at all. I really like the way this all works for changing host, the work you've done here and on access over the year has really paid off in making registration_change_host clean and well integrated.

    I've left a lot of small comments on the MR. Some bigger ones here:

    1. A single pipeline for both host and registration

    I think it gives event subscribers a clearer situation to work with if they are operating on all the violations of a single pipeline, rather than the way RegistrationConstraint currently dispatches 2 pipelines and merges the result. We should be able to find a way to make this possible.

    2. Cleaner inputs for constraints

    Relatedly, the fact that constraint validators get passed an ambiguous input that could be a host or a registration makes them a little more verbose than ideal. We could do more work in RegistrationConstraintValidatorBase to help with this. For example, RegistrationConstraintValidatorBase could provide a validate() method that calls out to validateRegistration(RegistrationInterface $registration, HostEntityInterface $host), validateHost(HostEntityInterface $host) methods together with empty implementations of those. Validators would then implement one or other of the 2 methods, and have trusted inputs to work with. It might even be possible to avoid the need for the dependencies or endPipelineEarly mechanisms by making a lot of use of this base validator class, but that's not straightforward to do.

    3. HostEntity::validateRegistrationForHost()

    I suggest we add a validateRegistrationForHost() method to HostEntityInterface. This has 3 purposesL
    (a) it's a reasonable API question - is this registration with all its registratin-related values valid for this host given the host settings?
    (b) we can move all of the logic out of RegistrationConstraintValidator completing the pattern of having validators call HostEntity methods.
    (c) I can use this method in registration_change_host to validate that moving a registration to a host is possible without actually saving the registration.

    If we do this, then we can add 2 protected helper methods to HostEntity: getAvailableForRegistrationPipeline() and getIsEditableRegistrationPipeline() that return arrays of constraints; then call either or both of these 2 helpers from HostEntity::isAvailableForRegistration, HostEntity::isEditableRegistration() and HostEntity::validateRegistrationForHost() (which merges the 2 pipelines before running them). It's a granular but DRY system.

    4. Remove isEditableConstraint

    If we do what I suggest above, then there's no need for isEditableConstraint, we can break it down into its granular pieces.
    - splitting HostIsOpen constraint into HostIsNotBeforeOpen and HostIsNotAfterClose
    - moving the admin access logic into HostEntity::getEditableRegistrationPipeline()
    - in HostEntity::getIsEditableRegistrationPipeline() instead of isEditableConstraint use a combination of HostIsEnabled and HostIsNotAfterClose
    - HostEntity::validateRegistrationForHost() doesn't call HostEntity::getIsEditableRegistrationPipeline() for a new registration.

    5. Causes should be translatable, but not codes

    I don't understand much about Drupal's translation system. But I'm sure constraint messages must be translatable. And causes should be. But codes should not be. Are we setting this up?

    As causes are translatable, should they contain punctuation. Punctuation is highly language variant as I understand it - not all languages use periods as separators. But in registration_change_host I concatenate causes for display: "Disabled. Closed. No room.". Does this mean that the causes should end in periods, or is it the concatenating code's job to worry about language appropriate separators? I don't know a Drupal API for that so I think maybe causes should end in periods?

    6. Create permissions contraint

    We're actually missing a constraint, because there was a validation hole in the previous system. We don't validate that the current user has permission to create a registration of the type (self, other or anonymous) that they have selected; we rely on RegisterForm::alterRegisterForm() to build the form correctly. Seems to me like we should validate this as well. This will help me in registration_change_host.

    7. Current user

    When checking access, Drupal & symfony in theory don't assume that the user that access is being asked about is the current user. You can pass any account you want. (This mostly isn't that useful in Drupal because currently we only have the user cache context which is concerned with the current user.) The same principle in theory would apply here: it's not crazy to ask if a certain registraion change would be valid for a certain user to make. I don't think we should put much effort into this now, but perhaps to prevent a BC problem in the future we should pass the current user into the validator and expect the constraint validators to call getAccount() on the context rather than themselves accessing the current user service.

    8. hasRoom

    I viscerally dislike what we do in hasRoom, passing in the current registration:

    public function hasRoom(int $spaces = 1, ?RegistrationInterface $registration = NULL)
    

    Perhaps a better alternative would be Registration to have a getAdditionalSpacesRequested() method that returns the difference between the current spaces requested in the registration and the spaces requested in the previous saved registration (allowing for the prevous and current state, etc). This might make quite a bit of this logic easier to understand.
    Perhaps this should be a follow-up, but we're touching on this quite a bit here.

    9. isNewToHost()

    Often the code calls isNew() when what it really means is isNewToHost(). We haven't cared about the difference before because changing the host hasn't been considered. Probably this should be a follow-up.

  • 🇺🇸United States john.oltman

    Thanks for all of your great feedback @jonathanshaw - I've replied on the inline MR comments, and will reply to your bigger items 1-9 within the next day or so.

  • 🇺🇸United States john.oltman

    1. A single pipeline for both host and registration - agreed, will fix

    2. Cleaner inputs for constraints - HostAllowsSpacesConstraint is the only one left like this - I'll fix it

    3. HostEntity::validateRegistrationForHost - I'm on board although the name should probably be validateRegistration, or even better just validate and it can take a mixed - out of the box it will only validate registration entities, but I'll fire an event so developers can validate other things.

    4. Remove isEditableConstraint - Admin access has to be in a constraint so it can add to the cacheability.

    5. Causes should be translatable, but not codes - messages are translated by passing the DrupalTranslator class to Symfony - but it does not translate anything else, so we have an issue with causes. We should add periods to the causes. But we'll have to translate those on the way in via ->setCause(t($constraint->disabledCause)). I can fix.

    6. Create permissions constraint - I assume this would be one of the "at save" constraints? Because on entry to the form we may not know what flavor of new registration the user will create.

    7. Current user - The only places we leverage current user in constraints are RegistrationIsEditable and UniqueRegistrant. In the former we are already passing the account - in the latter, we actually need current user since the messaging changes. This is probably a "won't do" for now.

    8. hasRoom - follow up issue.

    9. isNewToHost - follow up issue.

  • 🇺🇸United States john.oltman

    I responded to all comments @jonathanshaw - let me know on the open items, then I can start making changes to the MR branch.

  • 🇬🇧United Kingdom jonathanshaw Stroud, UK

    From #30

    3. HostEntity::validateRegistrationForHost - I'm on board although the name should probably be validateRegistration, or even better just validate and it can take a mixed - out of the box it will only validate registration entities, but I'll fire an event so developers can validate other things.

    I don't really get the idea of a HostEntity::validate() that takes a mixed. The pipeline that it is executing is a pipeline that is all about validating a registration fits a host. Without a registration, most of the constraints in that pipeline make no sense.

    4. Remove isEditableConstraint

    I keep finding myself uncomfortable around isEditableConstraint and HostEntity::isEditableRegistration(), but I haven't done a good job of figuring it out. I've gone round and round and I'm defeated by it actually!

    Some additional points I've discovered:

    A. Previously validation allowed admins to automatically override custom errors. I don't see that as a good thing, but it is a behavior change.

    B. The 'Set and forget' feature would seem to become irrelevant once we land this.

    C. We should find a way to use trigger_error to deprecation subcription to the legacy event. I don't think that deprecating the event constant will be enough to warn developers to move away from relying on this event.

  • 🇺🇸United States john.oltman

    I don't really get the idea of a HostEntity::validate() that takes a mixed. The pipeline that it is executing is a pipeline that is all about validating a registration fits a host. Without a registration, most of the constraints in that pipeline make no sense.

    It will be something like this:
    
    if ($value instanceof RegistrationInterface) {
      // execute the code currently in RegistrationConstraintValidator
      $validation_result = (...);
    }
    
    // Fire an event that allows other types of values to be validated.
    
    // throw an exception if validation_result is still not set
    
    return $validation_result;
    

    You would call it from change host:

    $validation_result = $host_entity->validate($registration);
    

    One of the reasons I am trying to make this function "soft" is I could see it being used later, as the registration module evolves, for other things, like validating RegistrationSettings etc. I don't see any downside, and easy to do now while we are reworking things. Think of it as the validation equivalent of hook_entity_update which takes a generic entity.

  • 🇺🇸United States john.oltman

    I think we are aligned enough for me to proceed with changing the MR. We can then see where things land and go from there.

  • 🇺🇸United States john.oltman

    I'll keep MR threads unresolved until we're closer to merge.

  • 🇺🇸United States john.oltman

    Ok this is ready to merge. Feel free to take another pass to make sure no show stoppers. Then I'll merge and you can finish off change host. I'm out on vaca the rest of the week after today, so most likely will not be able to review change host until the weekend. I know there are some nits but pretty sure they can all be handled in follow ups.

  • 🇺🇸United States john.oltman

    Targeting March or April for next release, to give plenty of time for follow ups and the other access/validation issues still outstanding.

  • 🇬🇧United Kingdom jonathanshaw Stroud, UK

    Still reviewing, will look more tomorrow ...

  • 🇺🇸United States john.oltman

    Awesome, thanks for all the issues and the roadmap!

  • 🇬🇧United Kingdom jonathanshaw Stroud, UK

    ->setCause(t($constraint->notOpenYetCause))
    RegistrationConstraintBase could instead in its constructor do:

            foreach (get_object_vars($this) as $property => $value) {
                if (is_string($this->$property) && str_ends_with($property, 'Cause')) {
                    $this->$property = $this->t($this->property);
                }
            }

    Not sure if that's a good idea.
    Also might be possible to handle the translation automatically in RegistrationExexcutionContext.

    I have slight concerns about HostEntity::validate() and what the API surface should be there, but at this point (given our plans for no immediate release) it seems to me there's an overwhelming advantage to committing this as is and iterating in more targeted follow-ups. I'm confident this is so close to being right that the follow-ups will not be too entangled with each other, and entanglement would be the only argument for further iterating in this MR.

  • 🇺🇸United States john.oltman

    Committed to 3.3.x branch. Didn't cherry-pick to 3.1.x branch since I'm not clear on plans for 3.4 (we can't do a 3.2). Hopefully you can switch your issue for change host to 3.3.x and then open a new branch for your work there, on top of 3.3.x instead of 3.1.x. Will catch up with you end of the week.

  • 🇬🇧United Kingdom jonathanshaw Stroud, UK

    Additional follow-up issues created:
    - 📌 Add a Registration::isNewToHost() method Active
    - 📌 Explore alternatives to passing existing registration into hasRoom Active
    - 📌 Add a register permissions validation constraint Active
    - 📌 Improve maximum spaces validation Active
    - 📌 Finalize the HostEntity validation API surface Active

    The draft change record needs some work before the release. The only behavior change I'm aware of is that previously validation allowed admins to automatically override custom errors. I don't see that as a good thing, but it is a behavior change.

Production build 0.71.5 2024