🇦🇹Austria @klausi

merge request created, uploading stable patch fiel for composer patches.

klausi → created an issue. See original summary → .

klausi → created an issue.

Turns out the remaining LoigicExceptions in code are correct, I don't think we can remove them.

I merged this now - we can do anything that is unclear or could be improved in follow-ups. Better we get this big change out of the way now so that we can work better on other improvements.

Merged, thanks!

@Andriy: I pushed some changes to the pull request, we can improve the AlterableSchemaTest to test the actual AlterableComposableSchema class.

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

Thanks, this will be merged soon.

@adamps: Thank you! Can you make a new release?

Currently we have the 4.x-dev version in composer, but Simplenews Scheduler does not like that when there is no version in Simplenews.

Thanks - sorry we don't provide compatibility guarantees for test code, downgrading to normal.

I think we can add those methods, but be aware that we don't have test coverage for this test code, so it could break again in the future. But I think that is ok, it is just test code afterall.

Thanks, uploading stable patch file for composer patches.

Looks good to me, thanks!

Uploading stable patch file for composer patches.

Looks good to me, thanks!

Uploading stable patch file for composer patches.

I also fixed another deprecated warning in the tests, I think this can stay RTBC.

Also uploading stable patch file for composer patches.

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

I pushed the empty array as default value instead, please review!

Also attaching stable patch file here for composer patches.

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

Merge request updated. Uploading stable patch file here for composer patches.

I think the tests are still missing here.

Thanks, uploading stable patch file for composer patches.

Thanks for reporting!

This happens when you have no blank line after the PHP open tag. Somehow the Squiz standard confuses Drupal file comments with the function comment. But it does not matter, you can fix that by having a blank line before the file comment.

Having a blank line after the PHP open tags is defined in our coding standards, for example https://www.drupal.org/docs/develop/standards/php/php-coding-standards#s... →

So I think there is nothing to fix here.

PHPCS 3.12 was released that contains my fix, I bumped the version in Coder's composer file.

Sure, but those annotation changes are not breaking changes? So we could do the them anytime later as well?

But if you would like to work on them I'm happy to include the conversions!

Updated the merge request. Uploading stbale patch file for composer patches.

thanks, uploading static file for composer patches.

Uploading static patch file for composer patches.

jjchinquist → credited klausi → .

This will be merged soon.

klausi → created an issue.

Unfortunately this is quite a big change, the important thing is to review the interfaces so that they are solid.

Merged into 5.x.

we will do this in 5.x

klausi → created an issue.

klausi → created an issue.

Automated tests passing now.

Now finally done.

Thanks, this will be merged soon.

vrancje → credited klausi → .

klausi → created an issue.

Merged.

klausi → created an issue.

Thanks, I cleaned up the rest of the code and improved the test code.

This will be merged soon.

vrancje → credited klausi → .

There is a deprecation incompatibility with 🐛 Cache collision when multiple servers are using the same schema plugin Active , opened a follow-up merge request.

Hm, not sure I understand the problem ...

graphql_composable is an example module for demonstration purposes, so it should not be scanned if it is not enabled. Maybe we have a bug that uses the schema files from disabled modules? Or you include the example module by accident somehow?

Fixed in 🐛 GraphQL Core module is missing when trying to install the GraphQL module in Drupal 11 Active , thanks for reporting!

Merge train is running, thanks for reporting!

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

Alright, added to the merge train! Thanks!

Merging the info file improvement.

Thanks for reporting!

1) There is no graphql_core module. Did you mean graphql? Should be "drush en graphql"

2) The route in the info file is wrong, we can fix that!

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

Added to merge train, will be there soon. Thanks!

Thanks, approach looks good!

I'm thinking about if the API addition could break existing installations - I think it will break all old sites that have dataproducer_populate_default_values set to FALSE. We could allow the parameters to be nullable with the "?" operator, which is ugly but should work?

I think you can go ahead and add tests now!

vrancje → credited klausi → .

I see, thanks for clarifying! Then I will close this for now and we can reopen or make a new issue in the future if this becomes relevant.

Thanks - I think we should only do the PHPCS fixes here, to not make the merge request even bigger with out of scope changes.

PHPStan: 📌 Fix the issues reported by phpcs Needs review

Fixing link in issue summary.

Merged.

klausi → created an issue.

Thanks a lot Jonathan for fixing this in 📌 Add new dictionary to cater for common words not in core dictionaries Active .

I don't think there is anything left to do in Coder, so will close this again.

Thanks for the suggestion!

Can you describe in the issue summary why we would need this for Coder? For checking changes of a Coder merge request I can checkout Coder locally and run it? Or what am I missing here, how would you apply Tugboat to test a merge request?

Another restriction is that we develop Coder at Github https://github.com/pfrenssen/coder . So we don't use/run drupal.org merge requests.

Attaching stable patch file for composer patches without the use statements in the meantime.

Thanks, I think the merge request should only change the nullable types compatibility, but I also see use statement changes? Is that on purpose?

I fixed the comments in the pull request, please review.

Uploading stable patch file for composer patches.

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

Also uploading stable patch file for composer patches.

klausi → created an issue.

Is this a security issue? If yes please report privately at https://www.drupal.org/project/simplenews/report-security-issue →

Uploading stable patch file for composer patches.

Closing this as duplicate of 🐛 Fix PHP 8.4 implicit nullable deprecation Needs review . Let's continue there!

Uploading stable patch file for composer patches.

Thanks for testing, merged!

I updated your merge request by replacing all the static calls in TaxonomyManagerTree. It is bad practice to repeat the class name all over the place, then any extending class cannot selectively override methods.

I also updated the core version requirement to 10.3.

Let me know if this works for you! I want to make a new release soon.

I don't fully understand what is exactly broken for you - can you post the error message?

The old issue was broken because the annotation "@FormElement" was changed by accident. That was not done here.

Merged.

klausi → created an issue.

Was fixed with 🐛 Restore term clicking AJAX functionality Active .

Merged manually, thanks!

Will fix the broken pipelines in a follow-up.

Thanks, PHPunit build tests are failing, but seems unrelated?

I think this is not possible in PHPCS or Coder, as it does not build a full function reference list of the code base.

PHPStan does that and can also parse comments, so I would suggest to open an issue at https://github.com/mglaman/phpstan-drupal and try to implement it there.

Thank you!

Looking good to me, I think we are ready to commit.

We now have a new approach for the AJAX functionality in 🐛 Restore term clicking AJAX functionality Active that works with multi-value fields and media libraries.

Please test the merge request there. If there are no objections I'm planning to merge it in a couple of days.

Thanks @torfj for working on it so hard!

Nice, looks good to me! Please fix the coding standard issues, otherwise I think we are ready to merge. The CI pipeline is failing, but that is independent of this issue and we can ignore here.

Marking this as RTBC.

@vladimiraus I would wait a couple of days if there are any objections and then merge this.

Hm, but that is a class constant. Of course class constants MUST NOT be prefixed with the module name as that would make them unnecessarily long. Class constants are already tied to a class with namespace, so no prefixing necessary.

The sniff Drupal.Semantics.ConstantName is about legacy global constants in the global scope that must not be used in modern Drupal anymore. (They cannot be autoloaded for example as far as I know)

We could add a new sniff to forbid global constants all together? Would probably make sense to open a new coding standards issue for that if we don't have it yet.

Does that answer your concern? Then I think we can close this.

Thanks!

One issue I found is that when you save a term it redirects you to the term page. But I would expect that I stay in Taxonomy Manager. Should be fixable by setting the correct form state redirect.

I did not test the media library integration - does that work as well that there are no AJAX errors? Then we could avoid introducing a setting to switch on/off Ajax mode as @vladimiraus suggested.

Thank you!

I can haz new release?

I'll get you a drink at Mountaincamp :)

Thanks a lot, nice progress!

Left some more small comments on the PR that we should address.