🇺🇸United States @owenbush

This took a while to track down, but what is interesting about the error you get and referenced is that code is not actually in Drupal core yet.

It seems to be part of this issue 🐛 [PP-1] bundleFieldDefinitions() are not added in EntityViewsData Needs work in patches up to, and including, #28. But this code has changed since that patch (to get it to work with D10/11), and has not been merged to core.

So it seems like something somewhere is applying that patch to core, and that is inadvertently breaking this functionality.

Is that something in your composer.json file, or perhaps in one of the composer.json files associated with Opigno?

Do you get this error on all pages or only on views pages? The patch I linked to fixes a bug when an inherited field is added to a view, because inherited fields are computed fields and Drupal core did not support computed bundle basefields in views until that patch merged.

But if you were not on a view page, then there may be something else at play. Can you give steps to recreate the bug?

Thanks.

This looks good and cleaned up a lot of unnecessary code, thank you

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

This has been fixed, thanks for the issue and the review.

Hey there,

I suspect, based on your versions, that this is actually the following core issue 📌 Allow adding computed bundle fields in Views Fixed , that has been resolved for I think 10.2+ but not backported to 9.x

I think this old MR that was targeting the 9.5.x branch should still work for you:

https://git.drupalcode.org/project/drupal/-/merge_requests/2511.diff

Thanks for the fix, this has been merged.

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

Thanks for addressing this.

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

This looks good, thank you.

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

Looks like the phpunit tests are now green.

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

This has been merged.

Thanks for this I'll get this merged.

That is so odd, I followed those same steps and watched your video, but it works for me. Hmm. I'll need to debug some to try and recreate it locally.

This needs review.

owenbush → created an issue.

Unfortunately, I don't think this would be possible.

The title, and description fields on the Event Instance entities are actually inherited from the Event Series entity. If you removed the dependency on Field Inheritance you'd end up with Event Instance entities without titles/labels. So in JSON:API you'd likely end up either with no data for titles for events, or potentially even failures if the code expects a label key to be present.

Yea this is a problem. There's a core issue around computed fields in entity queries: 🐛 Handle computed fields in entity queries: throwing a helpful exception is better than a PHP fatal error Needs work but the solution there is to just throw and handle an exception, that won't make this work.

The workaround above worries me because I feel like it could break inheritance completely in certain circumstances. I may have to think on this one, maybe even to the point of creating a custom event instance reference field so we can control things better.

I'm pretty sure this is a similar issue that is plaguing the Recurring Events module.

In that case there's an entity type eventinstance which actually inherits it's title from a parent eventseries entity. So the title field is computed. This causes issues when using an entity reference field to reference an eventinstance, the entity query fails because the title field does not actually exist.

Drupal\Core\Entity\Query\QueryException: 'title' not found in Drupal\Core\Entity\Query\Sql\Tables->ensureEntityTable()

✨ Unable to reference event instances Active

What version of core have you upgraded to? I just upgraded my local to 10.2.4 and I do not see these issues.

The event registration field is effectively a date range item under the hood with some additional columns. My initial thought was that it was related to reworking timestamp fields to prevent the 2038 bug, but it doesn't look like that has landed yet: 🐛 Timestamp field items are affected by 2038 bug Needs work

I wish there was a way to identify which of the columns is problematic, in the past it has shown that, but from your error it doesnt seem to?

Hi @trichers, your use case is certainly valid. I thought about adding a permission for being able to edit registrants of event instances where registration was closed, but the logic is also used for adding registrations. So I opted instead for a new permission 'administer all registrants' which allows basically any registrant operation. So users with that permission can add/edit/delete any registrant regardless of if they are in the past etc.

So in order for someone to be able to add internal notes after an event the user's role will need to be assigned the new permission.

I've added the code to a merge request, and here's a patch link if you want to test it:

https://git.drupalcode.org/project/recurring_events/-/merge_requests/87....

This has been merged. Thank you to all involved.

I've attached a newer version of the patch that uses PHP's Null Coalescing Operator (https://www.php.net/manual/en/migration70.new-features.php#migration70.n...) to make these changes more simple.

Please let me know if this patch still fixes these issues for you.

Thanks for submitting the issue and patch. I'll take a look.

This has been merged into the dev 2.0.x branch.

Thanks for the patch. I'll get this merged.

This has been merged, thank you.

Thanks for this I'll get it merged shortly.

I'm wondering if it may be worth having another permission to allow someone to modify a registration, even if the current restrictions are kept.

It's been a while since this code was written, but it was deliberate to not allow changing of registrations for past events.

I don't recall why exactly, though.

But if the event's registration has closed, or the event is in the past, or there's no more space, updating registrations is not permitted.

Now I can't remember why, and would be interested to know if people felt this was the wrong approach.

/me wished I had commented the code when I wrote it with a reason why.

This has been merged into 2.0.x and will be part of the next release.

This looks good to me, thanks for the patch and the RTBC.

I'll close this out as it seems to be working for you.

Please try out a patch from the MR and let me know whether it works for you.

Thank you for raising this issue. I see the problem now. There is a function `recurring_events_views_views_query_alter` which alters the query for the registrant list view to ensure that it displays registrants for all instances in a series registration series.

The problem is that previously the view had an id of 'registrations' but that was eventually changed in a later release of recurring events to be 'recurring_events_registrations' so it was better namespaced.

The code in the function was not updated to use that ID.

I'll get that addressed for you.

This has been merged into 2.0.x and will be part of the next tag release.

Thanks for raising this issue and providing a patch. I'll get that merged shortly.

To answer your question about creating a Registrant type, you cannot create one directly, just like you cannot create an Event Instance type directly. If you create a new Event Series type, it will also create an equivalent (same name) Event Instance type and Registrant type. The idea is that if you create an Event Series type, then the Instance and Registrant types will match.

An example, if you created a 'Youth' event series type with a number of instances, those instances would be of type 'Youth' too. And any registrants for a 'Youth' series or instance will also be of type 'Youth'. This allows for creating different registration experiences and forms per type.

Great stuff! I am glad you got it to work.

This has been merged. Thank you.

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

Thanks for the patch, this has been merged.

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

You could approach this similarly to how you did it when a registrant was created in 💬 How to send a notification e-mail to the admin when a registration has been made? Fixed , but instead use the YOUR_MODULE_registrant_delete hook.

You can actually add instances to an existing series, either by cloning an existing instance (and updating the date/time of it), or by going to the event series page and clicking on the 'Add Instance' tab.

Thanks for the patch, this has been merged.

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

Thanks for the patch, this has been merged.

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

This looks good, thank you. I've gone ahead and merged it.

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

I've so far been unable to recreate this issue.

When you submit the form to edit a registration, does the page refresh at all? If not, do you see any browser console errors?

This has been merged.

This looked good, thanks for the MR. This has been merged into 2.0.x

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

There is also a patch you could apply which exposes all custom entity fields as tokens. If what you want to create a token for is a registrant field itself and not something custom then take a look at

✨ Consistent tokens support across all entities and fields Needs work

Ah yes, sorry. Great catch. I am glad it works for you!

Are you trying to change a registrant as an administrator, or as the owner of the registration? If the owner, are you doing this anonymously (using the URL with the UUID in it), or as a logged in user?

At the moment, there is no mechanism in place to send emails to an administrator when a registration is made.

If you wanted to be able to customize the message sent out in the Drupal admin, you could implement the following hook:

hook_recurring_events_registration_notification_types() to add a new notification type, for example 'admin_registration_notification'. See the recurring_events_registration.module file for how this is done.

/**
 * Implements hook_recurring_events_registration_notification_types_alter().
 */
function recurring_events_registration_recurring_events_registration_notification_types_alter(array &$notification_types) {
  $notification_types += [
    'admin_registration_notification' => [
      'name' => t('Admin Registration Notification'),
      'description' => t('Send an email to an admin when someone registers for an event?'),
    ],
  ];
}

Then you could probably implement hook_ENTITY_TYPE_insert() with ENTITY_TYPE being 'registrant', with something like this:

use Drupal\Core\Entity\EntityInterface;
 
/**
 * Implements hook_ENTITY_TYPE_insert().
 */
function YOUR_MODULE_registration_insert(EntityInterface $entity) {
  $key = admin_registration_notification';
  $to = 'your-admin-email@domain.com';
  $mail = \Drupal::service('plugin.manager.mail');
  $params = [
    'registrant' => $entity,
  ];
  $mail->mail('recurring_events_registration', $key, $to, \Drupal::languageManager()->getDefaultLanguage()->getId(), $params);
}

Good catch, thanks for the fix. Merged into 2.0.x

The accesscheck patch has been committed to 2.0.x

This has been merged in 2.0.x

I'm not sure why this would be happening, but the fix seems fair enough so I've gone ahead and merged it.

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

This looked good, thank you for your work and I thought the approach was really good.

Thanks, Andy. It looks to me like in some other circumstances some of these other fields can end up mismatching, so I went ahead and added a hook update to resolve those too. Good find on the revisionable though.

For example I had this locally:

So the hook update should update uid for those who may have already tried to resolve this, and then also those other mismatches.

Nice work, I tested this out and it worked well. Thanks for the tests too.

And yes, I agree there's a lot of repeated code. I wish I could go back in time and make it a bit, less crap.

Thanks for the patch and explanations. This has been merged into the latest dev branches.

owenbush → created an issue.

This is an interesting idea and I think the idea of adding a hook is a good one for this use-case. I do have some questions, though. In order to trigger the instance deletions, you must be updating something related to the recurrence pattern (either the type, the date, the time, the duration etc). The skipped events then will not match that pattern as they won't have been recreated. So how are you handling that sort of thing? Manually updating those events? Or is it not a problem that some of the event instances may be out of sync with the series configuration?

I think this must be a hangover from:

https://git.drupalcode.org/project/recurring_events/-/commit/6bdc2517395...

I'll have to think how to resolve this.

I wonder if something as simple as the change in the MR for this issue would help.

Basically when creating instances they should use the owner from the series.

Thanks for this issue, I noticed it broke the automatic selection of the end date for excluded dates, but worked for included dates. So I fixed that in the JS. Would you mind reviewing that it still works your end?

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

Ok. I'll have to check but I suspect that the instances use the current logged in user ID, which in the case of migration doesn't make sense. So maybe there's an underlying issue whereby we need to use the uid from the series. I'll have to check when I next can.

I experimented with a patch in the module's composer.json file, but it is really problematic for a number of reasons:

1. You cannot guarantee that a site is using cweagans/composer-patches
2. The patch for core differs depending on what version of core it is for, so we would end up having to have different versions of recurring events for different versions of core, just to support different patches, which is unnecessary - it can be up to consumer sites to use whichever patch they need for whichever version of core they are using.

I will update the project page to link to the patch like I have done for the field inheritance module.

Given there are still sites using the 8.x-1.x branch, I had to update it to bring it in line with 2.0.x

Ignore the last commit, I was getting 8.x-1.x in sync with 2.0.x

Comment deleted.

I have just tested the following:

Drupal: 10.1.2
PHP: 8.1.16
simplesamlphp_auth: dev-4.x
simplesamlphp/simplesamlphp: dev-simplesamlphp-2.1

I was able to successfully authenticate, and was able to set up the simplesamlphp attribute mapping for automatic role assignment without a problem.

This seems to work as expected without PHP errors/warnings/notices.

Unrelated to the Drupal side of things, the simplesaml admin UI stuff all worked as expected as well.

I'm going to move this to RTBC as it appears we have two confirmations now of successful tests in D10, this one and #91.

MR !4139 was just behind 11.x a little, I've updated the branch to the latest 11.x, it is now mergeable again.

I have updated both MR 4139 (11.x) and MR 4224 (10.x backport) to change the field ID from 'computed' to 'field'.

I had previously removed the 'bundle' part though, so I assume that was seen in MR 1864 which is old and not used (I wish I could close it, but I do not have the permission to do so).

Now I've addressed the issue with the views.api.php doc change I'm moving this back to Needs Review.

I believe this is an underlying core issue which is called out on the Field Inheritance project page

📌 Allow adding computed bundle fields in Views Fixed

I've been working hard to try and get it resolved, but as of yet it is not merged into core.

MR 4139 (11.x) has been updated after the review from Joachim.

MR 4224 (10.x backport) has been updated with the same changes.

Moving back to Needs Review.

Ok looks like we have green 11.x and 10.x MRs.

The 10.x branch differs from the 11.x branch in FieldTest and EntityField.

EntityField has an optional argument of the entity_type.bundle.info service, if the service is not passed the constructor will retrieve it from the container and triggers a deprecation error (suppressed) to warn that in 11.0.0 the service is no longer optional.

This also affected the FieldTest class which in the 10.x MR had to mock the entity_type.bundle.info service and pass it into the instantiations of EntityField.

Other than those differences the 10.x MR is a direct backport of the 11.x MR.

Marking this as Needs Review.

Due to the number of changes in the 11.x MR it made more sense to backport the 11.x changes to a new branch rather than try and update the 10.x MR and have to force push, so I guess that can be closed (although, I couldn't do it myself).

I also closed out the 9.5 MR. So ideally we would just have the 11.x MR and the new backported 10.x MR.

Thanks for the review, joachim. I think I've addressed the issues you pointed out in the 11.x MR

1. You were right, no bundle class was necessary, and as a result a bunch of other things changed - including no longer needing (maybe we never needed one anyway) the JSONAPI test, which I added to ensure the bundle info was represented appropriately with jsonapi, but now we don't have a test bundle we definitely do not need that.

2. I addressed the comment in the views api you mentioned.

Once the 11.x MR is looking good, I'll go ahead and work on the 10.x MR to get it in sync.

Thats helpful thank you so much. I had no idea about that process. So, to me this feels ready to review. If someone disagrees then it can be changed back. But for now I'll move this to Needs Review.

11.x is green which is great.

So a couple of questions are outstanding:

1. The 10.x branch is missing a couple of changes from the 11.x branch such as the dependency injection of the service in EntityField, and the separation of the computed base field tests and the computed bundle field tests. So, how should we approach that? Backport a new version from 11.x to a new 10.x MR, or attempt to recreate the changes in the current 10.x MR?

2. Do we need to ensure that the service dependency injection is backwards compatible in the 10.x branch? What about the 11.x branch? My thoughts are that the service should be optional on the 10.x branch, but maybe deprecate it being optional in the 11.x branch and make it required, and trigger an error if the service is not passed in as an argument.

3. Anything else we are missing to get this through?

The failures were kind of expected in the jsonapi tests as I didn't modify any of those. I'll get those looked at tomorrow.

Thanks for pointing out the phpstan config I could remove.

As for the difference between 10.x and 11.x MR I dependency inject the entity_type.bundle.info service into the EntityField class in the 11.x MR. I don't think the 10.x MR does that. The FieldTest class instantiates an EntityField object several times, so I had to pass through the service as well.

One option to get around that would be to make the entity_type.bundle.info service optional in EntityField, and if it is not passed, instantiate it in the EntityField::__construct(). I feel like I've seen this approach elsewhere in core. That would mean we wouldn't need to update the FieldTest class, but it felt like the right thing to do, to pass all the required services.