Skip to content

Space before colon required in phtml files when using alternative syntax for control structures? #118

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
hostep opened this issue Jul 28, 2019 · 3 comments · Fixed by #132
Assignees
Labels
accepted New rule is accepted enhancement Improvements to existing rules

Comments

@hostep
Copy link
Contributor

hostep commented Jul 28, 2019

Description

Hi

This is more a question or a start of a discusion around the fact that currently a space is required before a colon in phtml files when using the alternative syntax for control structures.

So currently Magento code standards expects us to use this format in phtml files:

<?php if ($something) : ?>
    <!-- do something -->
<?php else : ?>
    <!-- do something else -->
<?php endif; ?>

I'm wondering why this is required at this moment? Was there a reason for requiring it like this? Or did it just happen because it is the default setting of the PHP_CodeSniffer project?
Because it looks very ugly (personal opinion), other php projects don't enforce this syntax, a lot of php tutorials/samples don't use it, and it's also not part of any php coding standard as far as I'm aware?

I would rather see this format:

<?php if ($something): ?>
    <!-- do something -->
<?php else: ?>
    <!-- do something else -->
<?php endif; ?>

This could be changed with this option:
https://github.com/squizlabs/PHP_CodeSniffer/wiki/Customisable-Sniff-Properties#squizcontrolstructurescontrolsignature

I'm seeing a lot of files being changed recently in the 2.3-develop branch whereas in Magento 2.3.2 this syntax wasn't being enforced yet. So it feels like somebody made this decision to enforce this but I can't find the reasoning behind it.

Not sure if this would make a chance to get changed?
No big deal if not, just wanted to bring this up 🙂

Thanks!

@hostep hostep added the enhancement Improvements to existing rules label Jul 28, 2019
@chris-pook
Copy link

I agree with this, the additional space adds nothing really and is very inconsistent across the codebase.

@lenaorobei lenaorobei added the need to discuss Rule requires discussion label Jul 30, 2019
@hostep
Copy link
Contributor Author

hostep commented Aug 13, 2019

@lenaorobei: any updates here?

If this gets approved, it best happens before Magento 2.3.3 gets released, otherwise a lot of files will get changed to the other format and then again in 2.3.4 to this new proposal (if approved). It would be nice if all changes could happen in a single Magento release if possible.
This makes figuring out what changed in between releases easier as fewer files will have been updated.

Thanks!

@lenaorobei
Copy link
Contributor

lenaorobei commented Aug 13, 2019

@hostep I'm fine with this change, but unfortunately it can be done for 2.3.4 and later. We have code freeze for 2.3.3 now.

@paliarush are you ok with it?

@lenaorobei lenaorobei added accepted New rule is accepted enhancement Improvements to existing rules and removed enhancement Improvements to existing rules need to discuss Rule requires discussion labels Aug 14, 2019
magento-devops-reposync-svc pushed a commit that referenced this issue Nov 10, 2021
…heck-php-deprecated-function

Created Sniff to validate function without argument
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted New rule is accepted enhancement Improvements to existing rules
Projects
None yet
3 participants