magento / magento2

Prior to making any Submission(s), you must sign an Adobe Contributor License Agreement, available here at: https://opensource.adobe.com/cla.html. All Submissions you make to Adobe Inc. and its affiliates, assigns and subsidiaries (collectively “Adobe”) are subject to the terms of the Adobe Contributor License Agreement.
http://www.magento.com
Open Software License 3.0
11.56k stars 9.32k forks source link

Use short echo tags in phtml templates #107

Closed colinmollenhour closed 9 years ago

colinmollenhour commented 12 years ago

PHP 5.4 has made the short echo tag <?= always available regardless of the short_open_tag setting so this feature is now 100% future proof and using it in templates reduces quite a bit of unnecessary typing/characters. The feature has been in PHP forever, only before 5.4 it could be disabled in php.ini.

Proposal example before:

<dt><?php echo $this->getFoo() ?></dt><dd><?php echo $this->getBar() ?></dd>

Proposed change to:

<dt><?= $this->getFoo() ?></dt><dd><?= $this->getBar() ?></dd>

I'd be happy to tackle this by writing a script which can do the replacement with a regex. I think it won't be hard using a lookahead to make sure the replacement is only done when there is one statement between the open and close tags (detected by semicolons).

Note, I am not proposing changing all tags to short tags, just the ones that contain a single echo statement on one line.

If you approve I will start on the script. Actually I would probably use DAMit which would make it easy for everyone to convert their code if they so desired: http://code.google.com/p/dam-tools/source/browse/trunk/DAMit.pl

EDIT: For users of PHP versions prior to 5.4 it should be possible to force short_open_tag to "on" via .htaccess.

php_value short_open_tag 1
IvanChepurnyi commented 12 years ago

I would love to... But how about minimal system requirement of PHP5.3? PHP5.4 is not yet so popular...

Also initial public version of Magento was with short_tags, but because of issues with on most of installments with turned off directives it was changed to <?php echo ...

daim2k5 commented 12 years ago

-1 It think the default value is off for shortcuts

Schrank commented 12 years ago

I prefer short tags too, but Ivan is right, many Magento versions are still running on PHP 5.2

But the comments after me are right.

If all the things are updated, it is a good idea to look forward. Using all the cool new features in PHP 5.3 and 5.4 would be great!

Let's change it! +1

Vinai commented 12 years ago

Magento 2 is supposed to be open to embrace change and the future. It's better to change the minimal requirements to 5.4 in my opinion. Someone running Magento2 on 5.3 should have no problem enabling short open tags. The biggest benefit in my eyes is improved readability.

:+1: from me!

EDIT: In case you missed it, in PHP 5.4 the "<?= is now always available regardless of the short_open_tag setting." (taken from http://www.php.net/ChangeLog-5.php#5.4.0 in the General Improvements) We probably all agree that Magento should not be made with unspecialized shared hosts in mind. I don't see a problem with 5.4 as a requirement if the host is setting up a tuned Magento environment anyway.

centerax commented 12 years ago

I totally agree with Vinai. +1

Flyingmana commented 12 years ago

+1 because a lot of templates I saw were not readable because of the overload of php code

johnholden commented 12 years ago

To play the devil's advocate... Zend forbids them in their coding standard: http://framework.zend.com/manual/1.12/en/coding-standard.coding-style.html

pavelnovitsky commented 12 years ago

On the other hand almost the same can be told about GD2, PDO, etc.

But any way -1

mzeis commented 12 years ago

I'd like the short echo tags for better readability but this shouldn't be the only reason to bump the requirements up to PHP 5.4. Make use of new features like traits and I'm in for 5.4. Another reason for PHP 5.4 would be to finally forget about register globals, magic quotes and safe mode.

As for support: Debian Wheezy e.g. will use PHP 5.4.4. That being said it will be a while before PHP 5.4 is widely used by providers. If you plan to release Magento within the next year, there may be problems regarding the PHP version.

Vinai commented 12 years ago

@johnholden It is PSR-1 compliant though: https://github.com/php-fig/fig-standards/blob/master/accepted/PSR-1-basic-coding-standard.md

Vinai commented 12 years ago

@Luge The XML parser compatibility problems where caused by the <? opening tag. <?= wasn't the problem.

benmarks commented 12 years ago

To johnholden's point regarding the proscription of short tags in ZF, that is for ZF1 which evolved when short tags might not be available. However, it seems that ZF2 will allow the use of short tags in view scripts.

LeeSaferite commented 12 years ago

+1 on short tags and +1 on 5.4 min version requirement.

As was already mentioned, a version bump to 5.4 would be great as you get things like traits which would be highly useful in Magento.

johnholden commented 12 years ago

@benmarks This is true, but it appears that Magento 2 with be using ZF 1.11, from what I see here on Github.

That said, I'm not opposed to short tags--I just like to keep things consistent. I've spent the last 10 years removing/rejecting short tags because that's what most coding standards have been calling for.

Also there's nothing stopping any individual or agency from using short tags in templates if they prefer.

SchumacherFM commented 12 years ago

+1 for short tags and also for the 5.4! and +1Mio for ZF2 ;-)

barbazul commented 12 years ago

:-1: For short tags :-1: For 5.4 requirement :+1: For ZF2

Although I am afraid switching to ZF2 at this point might involve pushing the final release date even further. I'd rather have Magento2 running on ZF1 rather than not having it at all so..... :-1: for ZF2

nhp commented 12 years ago

+1 for short echo tag (no general short tags) Moving to 5.4 could be a bit harsh because of many providers will take their time to make the switch to 5.4 and that could become a show stopper for smaller shops that try to upgrade/install Magento2. In General it would be a good choice but as mzeis said not only for usage of short echo tags but maybe for general features. ZF2 should be the way to go even if it only is because of maintainability.

leesbian commented 12 years ago

:-1:

This shouldn't be a matter of opinion, it should be a matter of pragmatism (as should pretty much all decisions of this type)

Unless PHP 5.4 is a requirement for Magento2, it's a bad idea - it's just one more thing to trip people up when setting up servers or local development environments.

Don't forget, not everybody will be able to enable short tags if they're not using PHP 5.4 - perhaps they don't admin their servers, or it's a shared server (either internally, or via a hosting provider), or it could be a cloud image that is non-editable by the customer etc.

I've spent a lot of time berating developers who use short tags, and removing them from their code - it's bad practice - it doesn't conform to Zend coding standards (so will fail any CI coding standards checks) and also leads to the inevitable "but it works on MY computer syndrome" - all Bad Things.

Besides which, if you allow short tags in templates, you have to allow short tags everywhere for the sake of consistency - anything else would be an invented convention for the sake of it - another Bad Thing.

Anybody wishing to continue using short tags can always configure a text expansion macro in their IDE, the only remaining issue is readability of templates - I personally find it MUCH easier to read the long form

As for ZF1 vs ZF2 - the choice is a simple one - if ZF1 will become EOL within the expected lifetime of Magento2, then ZF1 should be dropped in favour of ZF2 now. Same argument for PHP 5.3.

Vinai commented 12 years ago

@nhp Probably the bulk of migrations will happen 2014 or later. Until then 5.4 will be much more common place then today. As said before, Magento shouldn't be hosted in general purpose environments anyway. There are plenty of cheap shared hosts that have Magento specific offerings. For those 5.4 won't be an issue, since they are maintaining a special stack for Magento anyway.

Using a merchant who wants to install in the cheapest shared host available isn't a good smallest common denominator.

This really is a chance to open the doors for new possibilities and break with backward compatibility. If 5.4 is chosen as a minimum requirement all the other PHP features it brings with it can (and will) be used by module developers, and who knows - maybe even the core team :)

Sooner (5.3) or later (5.4) the time will come when we will feel weighted down by supporting an ancient PHP version with Magento 2 :) Lets make that later.

pavelnovitsky commented 12 years ago

So discussion changed from "short echo tag" to "Make 5.4 as a minimum requirement"

Again :+1: for 5.4 and traits, :-1: for short echo tag

Vinai commented 12 years ago

@leesbian I think a few of your assumptions are a little flawed.

I'm with you regarding the point on pragmatism though, even if it brings me to a different conclusion :)

The argument regarding the setup of a Magneto dev host with 5.4 tripping up folks is unlikely. C'mon, we aren't talking about the "generic PHP scripter", but rather Magento Developers, who should be able to figure out a PHP version setup. "Scripters" messing around in a complex system shouldn't be of concern.

Not everybody being able to enable short open tags on 5.3 hosts is just as saying Magento depending on mcrypt is a problem. It's just a system requirement, nothing more. repeat-my-argument-against-inflexible, non-magento-specific-hosting-solutions

If (big IF here) 5.4 is the minimum requirement for Magento 2, then the "it-works-on-my-computer" argument won't happen, since short echo tags will always work. They can't be disabled.

The argument of having to allow short open tags everywhere if they are allowed in templates, well, we aren't talking about short open tags <? (which can be disabled), but short echo tags <?= (which are always on).

@Luge I agree that I'm (miss?)using this thread to push for 5.4. Still personally I also think that short echo tags are cool :)

leesbian commented 12 years ago

@Vinai your point about "it works on my computer" is exactly the same point I made, which is why I was supporting the move to a PHP 5.4 requirement.

However, I think the deal with people not being able to enable short tags on PHP 5.3 is massive - especially as we move further towards cloud environments such as ElasticBeanstalk where the customer has no control over the PHP runtime.

Magento users shouldn't NEED a "Magento specific" environment. There shouldn't be a NEED for "Magento specific hosting solutions". Those only exist at the moment because Magento is a bloaty resource hog - I would hope that Magento2 is addressing those issues - cure the disease, not the symptoms :)

Anyway - make PHP 5.4 a requirement, then it's up to the individual what they want to do - either method is valid, nothing breaks, no special setup required.

benmarks commented 12 years ago

I had not intended to inject the topic of ZF2 as a Magento 2 library in my earlier comment. I was merely using it as an example of an ecosystem-neighbor framework allowing short echo tags (ty Vinai for reminding us of the distinction). That discussion does not belong here. In general, I trust that the core team will balance their work and timelines against the expected lifetime of Magento2.

I read @leesbian 's comment and came to say much of what @Vinai just said. I'll add that, in response to Lee's point that "if you allow short tags in templates, you have to allow short tags everywhere for the sake of consistency", well - ZF2 is doing just this, and the reasoning is readability. Certainly this is as subjective a topic as "which brace style to use" and should receive input from us all.

Regarding PHP 5.4 and Magento 2: this is germaine (where ZF2 isn't necessarily) because it makes the use of short open tags in templates tenable. My $0.02 is that whereas PHP 5.4 was released March 1, 2012, and whereas Magento 2 will not be officially released for some time now, making 5.4 a minimum requirement is not only possible, it will prove to be timely and advisable on its own merit, regardless of the short echo tags issue.

colinmollenhour commented 12 years ago

Thanks for the comments, all.

I haven't tested but it should be possible to switch on short tags setting in the .htaccess file to circumvent issues with users or hosts clinging to 5.3.

barbazul commented 12 years ago

This dicussion is starting to feel a little like the namespaces separators one.

I personally don't like short open tags and find them counterintuitive and more difficult to read and/or detect than <?php echo

But @Vinai and @benmarks make a good point regarding pushing forwards for new versions and features.

I got me thinking... Magento IS (or should be) the number one ecommerce platform so it should definitely be considered as one f the game changers platforms hosting companies should be looking at when deciding what services to provide. Other software that falls into this category: Drupal, Wordpress, etc. The PHP community has always suffered from this "hold back" syndrome and software platforms of this should take the blame, not hosting companies. Nevertheless the release schedule for magento2 is still blurry and as far as we know they could release tomorrow (not likely) or in 3 years from now, so although I am changing my vote to :+1: PHP 5.4 compatibility (in case somebody is counting) i would not race to implement traits and short echo tags yet to keep the possibility open to change back to 5.3 easily in case of an earlier than expected release.

ZF2 Is a different story. @leesbian makes an excelent point. There simply is no other option than making the switch now. It's not a matter of opinion. It's a little extra work now to make magento2 live longer

:-1: For short tags :+1: For 5.4 compatibility (and future requirement) :+1: For ZF2

gfxguru commented 12 years ago

+1 on php 5.4 ... and I would like to know how to circumvent now. We've updated to 5.4 because of security reasons, bug fixes, etc. I believe an incompatibility with 5.4 is unacceptable. -1 short tags +1 ZF2

colinmollenhour commented 12 years ago

Does anyone have any real examples of respectable hosts that otherwise support Magento that don't allow short_open_tag? I find it hard to believe there are any or at least enough that it would warrant holding back on this change. I've seen several community modules that use short echo tags for Magento 1.x and I haven't ever heard a complaint about it. If there truly was such a situation that one's host refused to allow short_open_tag (is this even possible?) then a simple command could change them all back (including any third-party modules):

$ find app -name \*.phtml -exec sed -i 's/<\?=/<?php echo/' {} \;

How to then copy all of these modified files back up to the said host using highly insecure FTP is an exercise left up to the fool who would use such a host in the first place. :-P

Some stats:

Holy arthritis, batman! That's a lot of highly verbose and redundant characters!

For the record this proposal:

The biggest downside to this proposal is that with all of the time saved not typing "php echo" the Magento 2 team would have to fire one of their template designers. ;-)

leesbian commented 12 years ago

@colinmollenhour do you have button-phobia or something? ;)

Saving a few keystrokes isn't a particularly strong argument, especially when creating a text expansion macro in your IDE would have been less keystrokes than your last reply, wouldn't have required anybody to make changes to their server configuration, and achieves the same end result.

The PHP 5.4 issue has been raised because it plain DOESN'T MAKE SENSE to implement the short echo tags if it will cause more issues than it solves. Making PHP 5.4 mandatory for Magento2 ensures there is NO risk by implementing your suggestion, as it will ALWAYS work. After the text expansion solution, this is the simplest, most pragmatic solution. Just as @Vinai pointed out - PHP 5.4 then becomes a REQUIREMENT of Magento2.

The ZF2 issue has been raised because in an effort to justify short tags some people have mentioned future-proofing. We've already noted the ZF1 will become EOL well within the expected lifetime of Magento2, so for a consistent argument, we also need to accept that ZF2 is required in order to future-proof.

This then circles back to the PHP 5.3 v's PHP 5.4 debate - as it's highly likely that PHP 5.3 will be EOL well before the end of the expected lifetime of Magento2, and as PHP 5.4 is GA now - and offers the benefits of traits (that would HUGELY benefit Magento) - why would we not make the decision now to mandate PHP 5.4?

This then circles back to the whole LTS conversation - because at the moment, we can only guess at the intended lifetime of Magento2 (I'm assuming a minimum of 5 years, as it's common for implementation costs [for large projects] to be amortised over a 5 year period).

colinmollenhour commented 12 years ago

What about maximum line length? The adopted rule is soft max of 80 and hard max of 120 although I very frequently see lines past 120 in Magento. Saving 7 characters per echo means fewer lines will exceed the maximum line length and need to be broken or the ones that still exceed will not be exceeding the maximum by as much. Look at any template file that has lines exceeding 80 characters and 95% of them will have at least one echo tag. Nothing hurts readability more than not being able to see the code on your screen!

As for requiring PHP 5.4 due to the impending EOL of PHP 5.3 this is a moot point since PHP 5.4 is still obviously supported. In the case of ZF2, as we have all seen with the xml security hole, if there is a security hole found Magento will provide the patch well before Zend does anyway so I really don't see a need for ZF2 as it will probably introduce more problems than it fixes.

colinmollenhour commented 12 years ago

Visually compare php echo to short echo (catalog/product/price.phtml has 170 instances of '<?php echo'): Long: http://screencast.com/t/VJHu92RIt6FG Short: http://screencast.com/t/G0yhgk194I

rgranadino commented 12 years ago

@colinmollenhour I don't think the argument is really about which looks better, it's the broader issues / implications such a change would have.

I'm siding with "it depends on what the required version of php is" and let that guide the decision for this topic. Personally, I feel both 5.4 and ZF2 should be chosen. I don't see mage2 being released anytime soon unless they stick with ZF1 and 5.3, which has been pointed out may reach EOL during mage2's lifetime.

magento-team commented 12 years ago

Thanks everyone for the valuable input! We will discuss it and let you know.

leesbian commented 12 years ago

@mage2-team that's great!

Just so we're clear, the expected output of those discussions is :

The community - as a whole - has been waiting for these statements for months, so we're glad you've committed to providing these answers :)

barbazul commented 12 years ago

@leesbian @mage2-team No pressure, right?

petemcw commented 12 years ago

@leesbian Thanks for the very clear re-cap! The community would benefit from having those answers.

leesbian commented 12 years ago

@barbazul @petemcw the point I was making is that as the discussion in this thread has shown, the decision regarding short echo tags can't be made properly unless the other issues that have also been mentioned here have also been considered

As these decisions are likely to have quite large implications for Magento2 going forwward, it's better to communicate these as soon as possible - and to make a decision (re: short echo tags) without communicating or explaining as to how that decision was made isn't going to inspire confidence that whatever decision has been made was made for the correct reasons :)

petemcw commented 12 years ago

@leesbian I totally agree. All these issues are intertwined to some degree. Having a community-involved discussion is the starting point. The sooner some of these decisions are made and communicated transparently lets us all know what we're in for with Magento2. Even if I don't agree with the outcome, I'd love to have a clearly communicated roadmap and documentation that provides answers the above mentioned points.

xxorax commented 12 years ago

:-1: for PHP 5.4, too young (or yes if you have planed to release Magento 2 on 2014) and so :-1: for short tags

For ZF2, it is young and it will causes many problems and have many missing components, but it will become the future and the development is very active. And as you said ZF2 is not a requirement for Magento2, and I think even new usefull module will be release in ZF2, why not implement them as other libraries ? ZF1 and ZF2 components can run both in the same script, so :-1: for ZF2 as a base of Magento2, but do not stay close to ZF2.

peterjaap commented 12 years ago

+1 for PHP 5.4 +1 for ZF2 -1 for short open tags

DanielSousa commented 12 years ago

:-1: For Short tags and 5.4 is to soon. :+1: for ZF2

monocat commented 12 years ago

Hi All,

Thank you to all who joined us last week to the Magento 2 developer live chat session. Few of the questions posted in this thread were covered during the session. Answers are listed below for your reference.

Will you be adopting PHP 5.4.x as minimum requirement for Magento 2? Currently Magento is not planning to use specific features associated with PHP 5.4.x and therefor we’ll not make it a minimum version requirement. For now, the minimum requirement remains 5.3. However, adoption to minimum requirement of PHP 5.4 is still under investigation.

Will you be adopting PHP short echo tags in Magento 2? Short tags are currently under investigation by engineering team. If we decide to incorporate PHP short echo tags in Magento 2, it will be supported either for PHP 5.3 (we do understand the possibile implication of 5.3 and short tags for shared hosting and will consider this in our decision process) or PHP 5.4.

Will Magento be migrated to Zend Framework 2? Magento will continue to support appropriate Zend Framework components as it is tied to the Magento application. Currently our team is evaluating the implication of migrating to the Zend Framework 2 and exact components that will be used with Magento 2. We will continue to support the Zend Framework that is integrated with Magento 2 in the future.

The Magento 2 Developer Team Live Chat recording and slides can be found at the following link: http://www.magentocommerce.com/media/webinars/magento2-live-chat/view

Thanks.

colinmollenhour commented 11 years ago

Just an update.. PHP 5.3 has hit EOL back in June/July (when PHP 5.5. was released) and will only receive critical bug fixes until June/July 2014 which is hopefully only shortly after Magento 2 will be officially released (WAG). So supporting PHP 5.3 means supporting a no-longer supported PHP version which is not a good idea for ecommerce stores where security vulnerabilities could be discovered and not fixed. Requiring a version of PHP that is over 2 years old (5.4.0 was released March 2012) does not sound unreasonable to me at all..

sshymko commented 11 years ago

@colinmollenhour I can confirm that PHP 5.4.0 is going to be set as a minimal system requirement for Magento 2. In fact, in the internal repository validation of the minimal PHP version has been already updated in app/bootstrap.php. All workaround for PHP < 5.4.0 have been removed from the code base as well. Those changes will become available in the public GitHub repo with the next code publication.

philwinkle commented 11 years ago

My understanding is that the use of Twig would preclude the need to have echo statements anywhere in the frontend 'templates'?

On Mon, Sep 23, 2013 at 11:56 AM, Colin Mollenhour <notifications@github.com

wrote:

Just an update.. PHP 5.3 has hit EOL back in June/July (when PHP 5.5. was released) and will only receive critical bug fixes until June/July 2014 which is hopefully only shortly after Magento 2 will be officially released (WAG). So supporting PHP 5.3 means supporting a no-longer supported PHP version which is not a good idea for ecommerce stores where security vulnerabilities could be discovered and not fixed. Requiring a version of PHP that is over 2 years old (5.4.0 was released March 2012) does not sound unreasonable to me at all..

— Reply to this email directly or view it on GitHubhttps://github.com/magento/magento2/issues/107#issuecomment-24930717 .

Follow me on Twitter - @philwinkle Friend me on Facebook - http://www.facebook.com/philwinkle

colinmollenhour commented 11 years ago

Thanks for the update, Sergey. Any word on short echo tags in phtml files?

I know there is Twig support, but are all templates going to be converted to Twig or will the core templates remain PHP?

sshymko commented 11 years ago

There are no plans to use short echo tags in phtml files for now. I personally would vote for that improvement as well. I'll go ahead and raise this topic in scope of PSR-1 compatibility we're working on right now.

Most probably all existing phtml templates are going to be kept, no Twig templates are going to be provided out of the box. However, ability to create your own Twig templates is already there.

mzeis commented 11 years ago

Most probably all existing phtml templates are going to be kept, no Twig templates are going to be provided out of the box. However, ability to create your own Twig templates is already there.

Off-topic: please stick with one solution: either phtml or Twig. I believe it will do more harm than good if you split up the community into one half phtml, one half Twig. More choices isn't always good.

On-topic: if PHP 5.4 is the minimum requirement and <?= is always on then I'm in.

SchumacherFM commented 11 years ago

Twig: +1 for @mzeis otherwise you will see in the phtml again a lot of business logic. it is too alluring to use incorrect and inperformant PHP patterns in the templates ...

michaelgrosser commented 11 years ago

Agreed with the above. A mix of template patterns is going to lead to a lot of the sloppy code and sloppy extension problems that exist in the Magento ecosystem (mainly 3rd party plugins) today.

If the decision is to embrace Twig, it should be embraced across the board and not selectively.

benmarks commented 11 years ago

+1 for standardizing on one template idiom. Swappable template types could possibly be a "nice-to-have", but I'm more than happy to have Twig be the One True Template Type®.