🇷🇴Romania @Andras_Szilagyi

Thank you @raveen-thakur for your fixes, I've taken care of the cspell

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

Andras_Szilagyi → created an issue.

Andras_Szilagyi → created an issue.

Think best would be to merge this as its green in 9.5, and then fix the tests in the groups upgrade ticket

Now that the d10 compatibility was merged (not ready) we will have to update and merge https://www.drupal.org/project/gcontent_moderation/issues/3309542 💬 Is group content moderation module compatible with Groups 2.0.0-beta3? Needs work without tests, or merge this and make the d10 tests pass in that issue

this was 2 early, first https://www.drupal.org/project/gcontent_moderation/issues/3348700 🐛 Fix failing tests Fixed should have been merged, then the group 2.x, then this

The 1 test is failing because this branch is not d10 compatible yet

MR created, there I will have the fix and test together, but I'm uploading also as patch to have a static fix until the issue is merged.
The parent issue should add the test, fail, and then this one would fix it.

Andras_Szilagyi → created an issue.

Actually reopeneing to permit the bot to post further patches

Nobody noticed the failing test in D10?

Andras_Szilagyi → created an issue. See original summary → .

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

My time is limited, but if you provide a MR I will review it and would welcome the contribution

I'm leaving the ticket status as is to receive any future patches.

I ran rector and tested manually in d10, works well, no errors in the logs

Re-opening to receive further patches from the bot.

I'm marking this as child of the D10 upgrade issue as it was necessary and fixed there, and closing.

Ready for you @ieguskiza

@iguskiza please enable test to run on fork

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

I've created a patch without the changes from #14, I see some d10 related stuff, doesn't make any sense to me.

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

Can one of the maintainers please enable testing on git push in forks, I need to add testing so everyone can see this pass in D9 and D10, see https://www.drupal.org/node/3297903/qa/merge_request/3297903-automated-d... → "There are no tests for this merge request yet. The first test must be triggered with a git push."

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

Shouldn't we reopen this in case the bot wants to post future patches?
Also I don't see one passing test, and I'm not surprised as the defaultTheme is set to classy which is gone in d10.
Please set back to active status, this needs more work. (I wont do it as I think its the maintainers decision)

Tested this with group 2.0.0-rc2 and the patch works as expected.

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

I confirm the issue is still present and that the patch solved it. I got the error using gcontent_moderation, same errors when visiting the group permissions page, the patch fixed it.

Andras_Szilagyi → created an issue.

I wanted my suggestion to be reviewed

I'm closing this issue as the child issues fixed it

The MR is ok and passes, and we can see the same test fail for the right reasons in the patch.

In symfony 6.2 the signature is:
public function encode(mixed $data, string $format, array $context = []): string
while in csv_serialization the signature:
public function encode($data, string $format, array $context = []):string

shouldn't we fix this?

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

Settings this back to active to get any future patches from the bot

Tested with rector and manually in d9.5 and d10, works as expected, also added php 8.1 with D10 automated so we see the MR pass in D10. Its all good for me.

I'm dropping 8.x as it doesn't make sense anymore and only adds issues

Adding a patch to be able to call the tests

the strange branch name gave me quirky git behaviour, needs to upload another patch

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

I made the patch to go forward but please do credit @escuriola

I'm uploading a patch, the reason is that I don't think the core constraints have to be changed in this issue, and for a reason I dont understand I cant push to the MR

Other than my question above it looks good and works

FILTER_SANITIZE_STRING used to strip also tags, current solution does not, but that's ok, because we can safely assume that there will be no html from the field labels.
From my tests this works fine, but if we want the automated tests to also pass then we need:
https://www.drupal.org/project/contact_storage_export/issues/3219887 🐛 Fix test for drupal9 Fixed

one of these 2 will need to go first though, up to the maintainer.

Fixed and also manually tested, works great, only thing, automated tests will work only with D9.1 and up now due to switch to olivero theme.

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

@Luke.Leber #4 probably didn't but I did after my changes, feel free to manually test the MR as well, your comment was useful and valid, but doing this would be a even greater help

That's a good point @mxr576, that's why I dropped everything below 9.3 in this latest commit

Please check, the patch is failing for the wrong reasons

Tests went well, drupal rector did not add anithing else to the changes.

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

I can't seem to add tests to run on the MR, and adding it as a patch would throw composer failure, so I ran them locally and also manually tested locally in both d9.5 and d10, all green all good.
For me it's ready for merge.

Running rector did not reveal additional changes

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

Uploading also as patch as I can't seem to add tests to my fork changes

The easiest way to reproduce the issue is to run the tests in d9

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

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

Hi @smustgrave, it's been a while but I remember trying to use the test_theme as a parent theme in one test in a custom theme, it threw schema errors. I also created a documentation issue that is related to this https://www.drupal.org/project/documentation/issues/3332588 →

Merged the bot patch with the outcome of drupal rector, also tested in d9.5 and d10 manually, all good, see MR

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

I've tested this manually in Drupal 9.5 and Drupal 10, works as expected and I also ran rector, no additional changes needed. For me the patch is ok for merging.

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

The bot patch seemed lacking, I ran rector and merged the result with the existing patch.

Andras_Szilagyi → created an issue.

Gave it a good round of testing and new empty behaviour is well covered in phpunit testing. I also credited the devs from the parent issue.

Thank you all!

Looks like it failed for all the right reasons, I'll merge the two in the MR

First we need to cover this behaviour in testing and see it fail, after we can see the two merged and pass.

Regarding the larger issue of how cache should be handled in this module I think we have to investigate in this issue https://www.drupal.org/project/facets_form/issues/3230828 🐛 Facets Form: Investigate potential facet dynamic cache issues Needs work , till then I think it is a needed fix.

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

smustgrave → credited Andras_Szilagyi → .

@proweb.ua unfortunately the entity translation picks up the schema changes only on module install or re-install, in effect you would not translate configuration via string translation using t() but (after reinstall) enable the translation for block configuration.

Patch with the parent's test (improved code but same in principle), to see it fail.

Andras_Szilagyi → created an issue.