Convert Announcements Feed to OOP Hooks

Some comments on the MR, mostly nits.

I applied two and answered the others I hope.

I'm going to try rector with a different setting on a new branch.

The MR for 3482567-rectorImportShortClassesFalse is ready for review. I know there are some coding standard issues, but this was run with a ddev script that runs almost all of the cleanup automatically.

The only manual bit I did was fix the hook function indentation.

This does need the array indentation fixed as well, but it fixes the \ on boolean null and some global drupal functions.

I think we'll need more opinion/consensus on which code style issues are fine to ignore for now. I'm personally fine with globally-namespaced functions (like \t()), but I think \NULL and \TRUE look really weird and unnatural. Not sure how to balance that with arrays not being multiline.

We figured out how to remove the \ I'll run it again later on a third branch.

We figured out how to remove the \

Check it out here: https://git.drupalcode.org/project/drupal/-/merge_requests/9923/diffs

We figured out how to remove the \

Nice! I think the outstanding question is whether the multiline arrays going away is fine.

nicxvan → changed the visibility of the branch 3482567-convert-announcements-feed to hidden.

nicxvan → changed the visibility of the branch 3482567-rectorImportShortClassesFalse to hidden.

New iteration where we fixed the attribute spacing issue.

nicxvan → changed the visibility of the branch 3482567-rectorImportRemoveLeadingSlash to hidden.

nicxvan → changed the visibility of the branch 3482567-rectorFixAttributeSpacing to hidden.

nicxvan → changed the visibility of the branch 3482567-testOnephpcbf to hidden.

This looks really good to me. I've pinged other committers for a +1 to make sure I'm not missing anything.

Note multiline arrays require a patch against rector because for some inexplicable reason they put a final on BetterStandardPrinter. I filed https://github.com/rectorphp/rector/pull/8875 currently the class is anonymous but perhaps we should ship EvenBetterStandardPrinter.

I can do a test where I convert everything to just to see what else pops up.

There were three tests I could not track down here https://www.drupal.org/project/drupal/issues/3482173 📌 Convert all test modules to use OOP Hook implementations. Active

I think one issue per module is the only realistic way to manage the reversing and manual patches I uncovered in the test issue.

The only problem with that now though is the php stan baseline conflicts each merge that will need to be regenerated which takes 2 minutes on my desktop and 30 minutes on my laptop.

Ideally the coder fix would get in too cause while that's one of the more straightforward fixes in my frankescript it ended up being inconsistent and I had to manually add that line.

I also created an issue on rector to remove final and they already closed it and marked it completed even though they rejected the pr.

https://github.com/rectorphp/rector-src/pull/6397#event-14895933824

https://github.com/rectorphp/rector/issues/8873#issuecomment-2439394430

I dunno but three failures in all test modules sounds like incredible to me. I can assist in fixing them. The current failures seems to be coming from asserting procedural implementations not actual bugs. If there are no more formatting concerns then that's near ready, isn't it? Perhaps we could fix those locally and then schedule a 24-48 hour period when no other core commits are made, generate a baseline, submit it and get all of that in a single go? Once that's done, converting all non-test should be easy shouldn't it?

If the core team has appetite for that I'm game.

I'll set up a full conversion test either way. There were a few test modules that do not match the standard pattern so they were not converted.

Now that formatting is down I can do a global convert and see if anything else shakes out. That is useful whether we group them or not.

Normally we try to schedule highly disruptive patches that touch a lot of files for a beta window, the next one is November 11-25.

I don't have an opinion yet on whether we should try to convert everything in one go yet, but it seems quite tempting just to get it all done especially if we can automate it and we don't run into trouble with tests or need many manual fixes.

I've been working through the test failures for converting tests, and I've been testing converting everything at once, there are some manual bits, but I am working on creating patches to automate applying / reverting the changes we need.

E.g there is a test that checks when .incs load and another that tests emply .module files, but those rely on hooks being procedural.

Left some comments on the MR - are we going to do DI and type-hinting while we're at it

I just toggled this back to ignore, thank you for your review though.

Short answer is the plan now as far as I can see is to do a one time lift and shift with rector. We will not be adding DI or type hints since it's essentially the same code, just executed from a different location, we're regenerating the baseline even.

This is being discussed as a one time move during the upcoming beta window assuming we get sign off.

These will now be smaller and more focused files so we can then create followups for typehinting and DI.

By doing it this way we're minimizing risk.

The issue with these two items is that both can uncover hidden items and require manual work and there is too much to do a one time move that involves manual work.

Since this is to be ignored just moving to NW so it doesn't rot in review queue