[Rector] Add Rector to CI for PHP upgrades code to PHP 8

[samsonasik]

This is:

• a bugfix
• a new feature
• refactoring
• additional unit tests

Checklist:

• Changes are covered by unit tests
  • Changes are covered by existing unit tests
  • New unit tests have been added
• Code style is respected
• Commit message explains why the change is made (see https://github.com/erlang/otp/wiki/Writing-good-commit-messages)
• CHANGELOG.md contains a short summary of the change and a link to the pull request if applicable
• Documentation is updated as necessary

Why this change is needed?

Hi, this PR provide a Rector to CI to for PHP upgrades code to PHP 8

[samsonasik]

/cc @TomasVotruba :)

[samsonasik]

Ready to merge 👍

[oleibman]

I like a lot of what this change is doing, but there definitely seems to be a problem. We don't depend on Scrutinizer as much as we did, but it still has some use, and this change seems to have broken it completely. 483 issues introduced? Issues like "The type PhpOffice\PhpSpreadsheet\Shared\Date was not found"? As a sanity test, I submitted a draft PR with a minor change, and nothing like that showed up for my PR, so it must be something in yours. I would need to see these "issues" eliminated, or at least explained, before I could consider moving this forward.

[oleibman]

It seems clear that Scrutinizer cannot handle some of the new syntax (e.g. underscores as group separators for large integer constants), at least not when running Php8.1. I am running another test to see if Scrutinizer does any better running Php8.3.

[oleibman]

Scrutinizer could not build the Php8.3 environment correctly. Trying 8.2.

[oleibman]

8.2 does better, but I'm not sure it's better enough. It flags the separator underscore as a syntax error, but at least it doesn't the class become unknown everywhere that it's referred to. Please try setting 8.2 as the Php version in scrutinizer.yml, and let's see the results.

[samsonasik]

@oleibman ok, updated 👍

[samsonasik]

scrutinizer error error seems unrelated with underscored literal number, thats on unknown class issue, I previously exclude literal number change before squash and enable it

[samsonasik]

@oleibman I excluded literal separator change fde5f55 👍

[samsonasik]

Ready to merge 👍

[MarkBaker]

I don't envy whoever chooses to review this PR. That's a lot of files changed, including style

[oleibman]

I have filed a bug report with Scrutinizer about the underscore separators - that's one of the changes I liked the best, and it would be a shame to not be able to use it. So far:

 We already had an internal issue for this, I've added your report to it and will keep you updated about its progress.

We'll see if anything comes of it.

[oleibman]

@MarkBaker @PowerKiKi I know it's a lot to review, but the changes are all fairly simple. I think it's similar in scope to other recent changes applied after eliminating support for Php7.4, e.g. moving typing from doc-blocks to code, or using MATCH in place of many-pronged IFs. Downside - a lot of changes with no real effect on functionality. Upside - a more modern-looking code base. Do either of you have any strong feelings on whether we should proceed with it if my review concludes that it ought to work?

[PowerKiKi]

This isn't great. It looks ugly, even more so with multiple parameters, and it is not supported by any IDE AFAIK. Instead it should be a proper @param annotation IMHO.

EDIT: I'd rather ignore the entire rector if it cannot produce adequate result, which is exactly what I did in the past, or fix it manually.

[PowerKiKi]

Rector is an awesome tool, but it also sometime produces subpar results (and rarely plain wrong). As illustrated by comment above. I am not convinced it is a good idea to integrate it into the CI as-is. I think I'd rather use it as a one-shot tool. I am wondering about the friction it would add for future contributions. Would it be acceptable ?

If we decide to have it in the CI, at the very least we should apply rectors one by one per commit, like I did. So at least it is easier to review. Once we added the entire list up to PHP 8.0, then we can switch the config to be the rector for PHP 8.0. And the code would stay intact for that particular commit. That way it would be a lot easier to blame a specific rector in the future if something went awry.

I am not exactly against this PR, but I am not entirely favorable either. I think it is somewhat risky, for the code, and for the workflow of future contributions.

[samsonasik]

sure, I will try create smaller PRs when I have a chance, thank you for the review.