Automated Drupal 10 compatibility fixes

Solved test errors.

Solved test errors.

Solved test errors.

Solved test errors.

Solved test errors.

@Serhii Shandaliuk, you should try running the tests locally, as these are clearly not working. Also, you should attempt to do an interdiff between your patches, so that we don't have to read the entire 52Kb of patch file to understand what you're changing between each attempt.

If you don't know how to do that, please open an MR and try using that way of proposing a patch.

I selected part of the patch from #8 that hopefully will work.

We can commit in multiple steps, each time making sure the tests still work for D9.5. The attached path is the parts I didn't commit so presumably it will still fail.

assertEquals() takes parameters in the opposite order than assertEqual() - the expected value is first. Although some of them were wrong in the first place😃. I feel we should swap them, unless it's unclear anyway which way round is correct. This will be a bit dull, but probably less than 1 hour. Ideally change to assertNull(), assertTrue() and similar also where there is a more applicable function. I'll be pragmatic - if on balance we made more things better than worse then it's good enough for me.

The last submitted patch, 23: simplenews.d10-readiness.3289673-23.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

There is a fatal error with sending with Drupal 10:

Error: Call to undefined method Drupal\Component\Utility\Unicode::mimeHeaderEncode() in Drupal\simplenews\Mail\MailEntity->getFromFormatted()

Unicode::mimeHeaderEncode seems to be removed from D10.

https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Component%21Util...

There is a fatal error with sending with Drupal 10:

Thanks for the report. Strange that the test results haven't already hit this.

Patches welcome.

Here's a patch for only SimplenewsAdministrationTest which was failing in the earlier patches, hopefully now fixed.

The last submitted patch, 28: simplenews.d10-readiness.3289673-28.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Here's the patch with the remainder - getting smaller each time😃

Great so the tests now pass. However that's not the end of the work, because in 📌 Fix D10 deprecations Fixed , deprecations were suppressed. There definitely are some deprecations, as pointed out in #26 and I saw some from the tests when running locally. I propose that the steps are:

1) As in #23 we should correct some of the asserts before committing it given that we are changing them. It should take less than 1 hour.

2) Create a patch to turn deprecations on again and fix whatever it shows. Or maybe easier: run the tests against D10.

If needed I will eventually do the work by myself over the next 6 months. However it would be nicer and much faster if others can help.

No need for deprecations, you can just add test runs against D10 as I've done above.

Plenty of test fails, but all of them are because most tests use classy, which is no longer in D10. Switch that to starterkit_theme and then we do another run.

@Berdir thanks for keeping an eye on simplenews still😃.

Switch that to starterkit_theme and then we do another run.

Good point, we could change the order round, and do my second point first, fixing all deprecations in the code. We can come back to my first point after, fixing the assertEquals before finally committing the test changes.

Switch that to starterkit_theme and then we do another run.

OK that I will do, however I've not got time to fix all the problems myself right now so hopefully others can help😃.

The last submitted patch, 36: simplenews.d10-readiness.3289673-36.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Hopefully these changes help. As I understand, iconv_mime_decode should be used instead.
Related CR: https://www.drupal.org/node/3207439 →

Fix : Access checking must be explicitly specified on content entity queries

Related to : https://www.drupal.org/node/3201242 →

+++ b/src/Mail/MailEntity.php
@@ -169,7 +169,7 @@ class MailEntity implements MailInterface {
     }
-    return '"' . addslashes(Unicode::mimeHeaderEncode($this->getNewsletter()->from_name)) . '" <' . $this->getFromAddress() . '>';
+    return '"' . addslashes(iconv_mime_decode($this->getNewsletter()->from_name)) . '" <' . $this->getFromAddress() . '>';
   }
 
   /**

You're changing this from *encode* to *decode*, that is the opposite and not what you want to do.

The correct replacement is using the header class from Symfony.

#36 shows that the theme change causes exactly one fail on D9 as well, something about the theme regions being empty apparently or maybe the path is wrong.

Additionally, there are a lot of fails because the test module is not marked as D10 compatible.

Sorry about that, now it should be fine.
There was one test that I didn't know how to change and I left it as-is just in case I mess up something :)

#39 is good thanks. It's only needed for content entities, so I believe we can remove 3 of them

Nice. Would be good to now handle also the mimeHeaderEncode deprecation, but on a much smaller patch than #41, please.

It's easier if we take one step at a time.

@rang501 Great thanks for #41. Please can you make a patch with only your changes (similar to the interdiff) and run tests against D9? Then you should see just one test fail, which you can fix by changing the test file to remove the quotes around Drupal.

For the last case where you aren't sure please post the code and someone can advise you.

Ok, here is the patch with only my changes.

The issue is with test line (SimplenewsSourceTest):

      // And make sure it won't get encoded twice.
      $this->assertEqual($from, Unicode::mimeHeaderEncode($mail['reply-to']));

This fixes (hopefully) the failed test. But the previous test question remains - not sure how to handle it.

Great thanks

The issue is with test line (SimplenewsSourceTest):

I see - that is a strange one. It seems to be a test that Unicode::mimeHeaderEncode() works correctly - that it won't alter an already encoded string. However we don't need to test the function of libraries that we use. I would just delete it, unless @Berdir can provide a better answer.

That was added in #1387648: Sender name is MIME-encoded twice → , 12 years ago, agreed that it's not our job to test that.

Thanks for feedback.
New patch is here

Great thanks @rang501.

Here is a re-roll of #36 which is likely to fail in the same way. It should be a fairly simple fix and it would be great if someone could volunteer. Run the test locally, look at the HTML output and then adjust the failing assert to match what's actually there.

The last submitted patch, 54: simplenews.d10-readiness.3289673-54.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Sorry bad reroll

The last submitted patch, 56: simplenews.d10-readiness.3289673-56.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Sorry another merge error try again

The last submitted patch, 58: simplenews.d10-readiness.3289673-58.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Two more easy fixes for the D10 fails

The last submitted patch, 60: simplenews.d10-readiness.3289673-60.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Fixes some more access checks

The last submitted patch, 62: simplenews.d10-readiness.3289673-61.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

OK that's me done for now - great if someone else can take a turn.

Ah I found one more patch I forgot to upload😃

The last submitted patch, 65: simplenews.d10-readiness.3289673-65.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Lets see if I got everything. The export error was a bit tricky, just got a client error without any information whatsoever, that's a bit weird. Turned out to be a change in InputBugs that no longer allow to access arrays.

verbose() I just removed, dump() is not a replacement really, it's just a debug tool that will fail tests. And the block fail was because that test needed to be updated to use the correct theme in the URL.

Amazing thanks @Berdir.

Here is a new patch based on #67, but excluding all the assertEquals() that I'd prefer to fix. Let's at least have then in a separate commit.

The last submitted patch, 72: simplenews.d10-readiness.3289673-72.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Hurray😃

Automatically closed - issue fixed for 2 weeks with no activity.