🇧🇬Bulgaria @pfrenssen

pfrenssen → created an issue.

Moved patch #11 to a MR.

I was wrong in comment #15, the original code does a loose comparison and it works as expected when comparing a string with a SimpleXMLElement object. But I still decided to keep this modest improvement in the MR ;)

The fix is incomplete. It is not sufficient to only cast $xml->file['source-language'] to a string when creating the error message. In the preceding if statement we also need to compare the string value.

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

Thanks for confirming, and for the great contribution! This is an awesome new feature :)

@milanbombschliip: there is a post-update hook that should take care of updating the media source. Didn't it work?

Yes it looks like Message Notify would need some work if we would go through with this. It is completely oblivious to language handling, it doesn't pass on a langcode when it renders the message in MessageNotifierBase::send().

I also think so, this has been fixed in #3307555: Message sent in interface language instead of set language as MessageTemplate::getText() is not fully respecting the $langcode parameter → . Feel free to reopen if necessary.

I am also confused by this. On the surface it looks like this is rather useless. We allow setting a language in non-persisted state on the Message entity just to avoid having to pass it to Message::getTexts()? Adding an additional layer of complexity for this seems absurd. Our users could just pass the $langcode to ::getTexts() directly. Why would we add an ephemeral property on the entity to avoid passing it?

There must be more to this story. I did some digging. The property and the getter have been introduced in this issue as part of the work to port the module from Drupal 7 to Drupal 8. At the time we had no experience using modern entities, and it looked like a great idea to introduce a __toString() method on the entity. It was probably envisioned that we could directly use the entity object in Twig templates, how cool would that be!

Now with hindsight and having years of experience with building Drupal 8+ sites it is crystal clear rendering entities is dependent on a view mode, and needs to be done using the EntityViewBuilder. Rendering entities as strings was a fun idea but it doesn't make sense in modern Drupal. AFAIK no other entity types in core or popular contrib modules implement __toString().

I propose to get rid of this. We should start by deprecating it, since there might be users in the wild who rely on this. Let's deprecate ::getLanguage(), ::setLanguage() and ::toString().

We should also check which code relies on this and will need to be refactored. We should take a look at the GetText Views field handler and other modules like Message Notify.

pfrenssen → created an issue.

pfrenssen → created an issue.

Thanks a lot for working on this! I had a look at the MR and it looks like the test failures might be significant, for example this one:

Declaration of Drupal\rdf_taxonomy\TermRdfStorage::__sleep() must be compatible with Drupal\Core\Entity\EntityHandlerBase::__sleep(): array in /builds/issue/rdf_entity-3434111/modules/rdf_taxonomy/src/TermRdfStorage.php on line 571

Did anyone have the chance to try this out on Drupal 11?

We need to check that newly added videos also get a thumbnail. So far I only tested with existing videos.

It looks like existing videos will not have their thumbnails updated, and it does not seem easy to do this in an update hook, because the Media::updateThumbnail() method is protected. There is an issue about this but it is still in progress: ✨ Expose triggering update of media metadata + thumbnail to end users Needs review .

Already done!

pfrenssen → created an issue.

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

Wow thanks for the quick response!

pfrenssen → created an issue. See original summary → .

Thanks for working on this, and for making a huge number of improvements! I reviewed the changes and they look good. I just have a question on the MR about the removal of a header text in a table.

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.