Skip to content

Conversation

frankverhoeven
Copy link
Contributor

@frankverhoeven frankverhoeven commented Oct 23, 2020

Allow the package to be used with PHP 8.
Upgrades Doctrine Coding-Standard from 7.x to 8.x, which includes PSR-12.

Notable breaking changes (from PSR-12):

  • Spaces are now required before and after . operator.
  • Multi-line if-statements must start on a new line, eg:
if ($foo->isBar() &&
    $bar->isFoo()
) {}

// Becomes

if (
    $foo->isBar() &&
    $bar->isFoo()
) {}

Changes in effect: https://github.com/MyOnlineStore/configuration/pull/209

Must be released as new major.

Copy link
Contributor

@digibeuk digibeuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't really like the removal of zero concatenation spacing, this would mean a lot of changes in the codebase(s) let's leave that like it is for the rest : 👍

@frankverhoeven
Copy link
Contributor Author

don't really like the removal of zero concatenation spacing, this would mean a lot of changes in the codebase(s) let's leave that like it is for the rest : 👍

It would certainly mean a lot of changes, but these can be done in a single PR by code sniffer. It is possible to automatically fix just a single rule, ignoring all other rules. In my opinion, spaces around the concatenation improves readability. Moreover, it also makes our codebase compatible with the PSR standard.

@digibeuk
Copy link
Contributor

don't really like the removal of zero concatenation spacing, this would mean a lot of changes in the codebase(s) let's leave that like it is for the rest : +1

It would certainly mean a lot of changes, but these can be done in a single PR by code sniffer. It is possible to automatically fix just a single rule, ignoring all other rules. In my opinion, spaces around the concatenation improves readability. Moreover, it also makes our codebase compatible with the PSR standard.

o.k. on second thought I will go for your suggestion to drop it, it is more in line with the coding convention from doctrine, the less deviations we have the better. We will add per-project changes if needed as I don't want to reformat entire codebases (again)

Copy link

@fred-jan fred-jan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (although my eyes are still burning from looking at that multi line if-statement)

@frankverhoeven frankverhoeven merged commit ebac83a into master Oct 28, 2020
@frankverhoeven frankverhoeven deleted the php8 branch October 28, 2020 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants