🇧🇬Bulgaria @pfrenssen

Sure! Done :)

I ported this to 3.0.x but now I am getting "This value should not be null" validation errors when creating new events.

The submission result has been implemented already. It is a bit different from the PoC, in that it offers a full WebformSubmission type where in the PoC it was only an ID and token. This saves having to do two queries to get the full data from a submission.

One thing that I see is missing from the PoC is the confirmation message. I have made an issue for this: ✨ Expose the confirmation message on a WebformSubmission Active .

Is there something else that is missing regarding the submission result?

pfrenssen → created an issue.

There are no coding standards violations at the moment. Closing.

pfrenssen → created an issue.

pfrenssen → created an issue.

pfrenssen → changed the visibility of the branch 3477047-allow-different-registration to hidden.

I'm going to move this to 3.0.x, this is now the main development focus.

Thanks for the fix!

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

This is already fixed in the 3.0.x branch as part of 📌 Fix constructors which have incorrectly marked arguments as nullable Active . Changing the order of the arguments is a B/C break which will affect existing projects which extend these forms with customized versions. That was why this was not fixed in 2.0.x.

I think the non-B/C breaking way to solve this is to make the trailing arguments nullable. Then the deprecation warning goes away, and extending code is not affected.

public function __construct(EntityRepositoryInterface $entity_repository,
  ?EntityTypeBundleInfoInterface $entity_type_bundle_info = NULL,
  ?TimeInterface $time = NULL,
  ?Messenger $messenger = NULL, // Make trailing arguments nullable.
  ?DateFormatter $date_formatter = NULL // Make trailing arguments nullable.
) {

I personally think this is not so important to backport to 2.0.x since it is just a deprecation warning and 2.0.x will be unsupported when D10 support ends, so I will close this as a duplicate of 📌 Fix constructors which have incorrectly marked arguments as nullable Active . However if someone deeply cares about this, feel free to reopen the issue!

We're using the regular DateRangeItem field type, you can get a DrupalDateTime object for the start time and end time which can be used to format the date range for the desired time zone. For example:

$event_instance = $item['content']['#eventinstance'];
$start_date = $event_instance->date->start_date->format('M j, Y g:i A', ['timezone' => 'America/New_York']);
$end_date = $event_instance->date->end_date->format('M j, Y g:i A', ['timezone' => 'America/New_York']);

I just checked how this works in core and the data is stored in the same format when I add a daterange field to a node type. I checked with the ISO 8601 standard and indeed, the date format used by the datetime_range module is ambiguous. The dates should have a Z suffix to properly indicate this is a UTC time.

I think the most relevant core issue is ✨ [PP-1] Add ability to select a timezone for datetime field Postponed .

pfrenssen → created an issue.

I had a quick look at the code but didn't try it out. This looks like a great addition, thanks for starting it! I have a few thoughts:

1. Since this uses Drupal 10+ attributes the MR should be rerolled against 3.0.x. The current 2.0.x still supports Drupal 9.3 so the code will not work on older sites that didn't update to Drupal 10 yet.
2. The code looks good as far as I can see. I compared it with how the node module does it and the approach looks the same.
3. I am not sure whether we should enable the creation of new revisions by default. At the moment this is not the case AFAIK. If we change this to be enabled, then we need an update hook that disables the revisions for each bundle of the event series and bundles. Then existing sites can decide whether they want to enable it or not.
4. Would be nice to have a small test, but that is not a hard requirement. We are mostly relying on core functionality.

Fixed the conflict and a new coding standards error that popped up.

Thanks a lot for the fix!

Thanks a lot for the contribution!

🐛 Tests are failing in 2.0.x Active has been merged, restoring previous status.

🐛 Tests are failing in 2.0.x Active has been merged, restoring previous status.

All green on Drupal 10 and Drupal 11!!

Thanks very much for the contribution! This will make it easier for the community to contribute to the module :)

Thanks for working on this! Unfortunately this can't be merged as long as the tests are not passing. Postponing on 🐛 Tests are failing in 2.0.x Active .

Thanks for working on this! Unfortunately this can't be merged as long as the tests are not passing. Postponing on 🐛 Tests are failing in 2.0.x Active .

pfrenssen → created an issue.

This has already been fixed in 3.0.x-dev.

Looks great! I will merge this. Probably we can close 🐛 Drupal 11: Views Data Alter hook clash Needs review now. I will have a look there.

@plopesc you're right, I still had this task on my to-do list, but in the meantime most of the issues are already solved!

I will also make sure the tests pass on Drupal 11.

Crediting @chris dart who did this work in 🌱 Roadmap to modernize code for D11 + PHP 8.3 Active .

I did a fresh check against the latest dev version and there is only one task left.

I created an issue to commit the D11 compatibility fixed that were proposed here by Chris Dart: 📌 Drupal 11 compatibility Active .

pfrenssen → created an issue.

I got PHPUnit to work by simple removing our custom phpunit.xml file which was not making DDEV happy.

Some PHPStan failures have popped up now, but these are not caused by this issue, but by some recent changes in the 3.0.x branch. Ref. 🐛 PHPStan failures in 3.0.x branch Active .

pfrenssen → created an issue.

This is super cool and helpful to start up a development environment for Recurring Events, thanks for starting this!

I am trying out the features. So far I had an error when running ddev phpstan but this is already fixed in the new release of the project.

For the moment ddev phpunit doesn't work. I'm getting PHP Fatal error: Uncaught Error: Class "Drupal\Tests\BrowserTestBase" not found in /var/www/html/tests/src/Functional/LoadTest.php:15.

I merged in the latest changes from 3.x and it turns out the main cause of the issue was fixed recently, in 📌 Fix phpstan and stylelint issues Active .

The current patch makes this a bit more robust, but probably the bug is already fixed in the latest 3.x-dev.

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

Thanks a lot for this and for sharing the process!

Wow very nice, thanks!

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

Looks good and solves the problem! I only updated the pipeline so the tests can run, so I think it is OK for me to RTBC this.

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

pfrenssen → changed the visibility of the branch 3435379-automated-drupal-11 to hidden.

pfrenssen → changed the visibility of the branch project-update-bot-only to hidden.

This looks good, the update to the info file is the only change needed.

This has been fixed a long time ago.

A big shout out to @Xano for suddenly coming out of the woodwork and adding me as a maintainer to the upstream commercie/currency library! I have merged the fix and issued a new release. Now our tests are all green, and this is ready for review again!

We can remove the big warning "Username save logic changed in 2.x" from the project page now that this is fixed.

We can remove the big warning "Username save logic changed in 2.x" from the project page now that this is fixed.

I'm impressed with the tremendous work that was done here, thanks @jeremy1606 and others! I updated the tests and locally they are all passing if I apply the patch from https://github.com/Commercie/currency/issues/8

I guess this is now postponed on https://github.com/Commercie/currency/issues/8 but the library seems to have been written by @Xano to support this module, and does not seem to be maintained any more.

We have a lot of deprecation warnings coming from one of our dependencies (commercie/currency). There is an upstream PR to fix these: https://github.com/Commercie/currency/issues/8

Let's not mark any tests as skipped. It is better to let them fail so we have good visibility on the work that is still needed. Marking tests as skipped gives a false sense of progress.

Since no Drupal 7 versions are still supported, this can be closed.

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

This has been fixed in the 3.0.x branch in 💬 Support youtube shorts 3.x Active . Let's make sure to include the unit test from that fix here too.

@chris dart I missed your contribution, thanks for starting the work on the D11 compatibility. However this is just a planning issue where we organize the bigger task of modernizing the code base. I think we might be ready now to start on the actual Drupal 11 compatibility. So far we were blocked on our dependencies (like the Field Inheritance module) not being ready. But we should do the work in separate issues and link them here.

Yeah I am guessing that users deciding to move to 3.x will probably be doing this as part of a D11 update. Thanks for the review!

The blockers have been solved!

Cool, all tests are passing both on PHP 8.3 and PHP 8.4.

Really nice to see this moving forward! I added a new blocker: 📌 Require PHP 8.3 in the 3.x branch Active

pfrenssen → created an issue.

Looks good, we only need to declare compatibility.

I did the remaining compatibility fixes, but it looks like this still needs more work. Some of the updates to the tests look like they might introduce new failures. I ran the tests and had a large number of warnings, errors and failures. A number of these probably are already happening in the main branch.

pfrenssen → created an issue.

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

pfrenssen → created an issue. See original summary → .

This has been committed a long time ago, and there have been no reports of any problems, so I guess it is OK to mark this as fixed. Thanks all!

Thanks, that is a very good suggestion. By extending the try-catch block we can rely on the exceptions, this is a better solution than filtering out NULL values. It will also catch any other incorrect data types.

pfrenssen → created an issue.

pfrenssen → created an issue.