Option to enable template hints on frontend with a configurable parameter #10608

DominicWatts · 2017-08-21T22:50:38Z

The recent addition to allow template hints to be toggled via the CLI is a step in the right direction as doing so via the admin is time consuming. However frontend development can be streamlined further with this additional feature.

Description

Within admin user is given option to enable template hints on frontend with a configurable URL parameter. So when enabled can pass ?templatehints=[parameter_value] to enable template hints providing they're enabled and the additional new option is enabled. Also for security the parameter value is configurable.

We often find ourselves in two situations

Dev 1 working on site wants template hints enabled, Dev 2 doesn't
Dev 1 on 1 screen wants template hints, on Dev 1 on screen 2 doesn't

This update easily allows for this.

Fixed Issues (if relevant)

N/A

Manual testing scenarios

Within Magento backend under Advanced | Developer
Enabled Template Path Hints for Storefront - Yes
Enable Hints for Storefront with URL Parameter - Yes
Configure Parameter Value - magento
On frontend Storeview enable template hints with parameter ?templatehints=magento

Contribution checklist

Pull request has a meaningful description of its purpose
All commits are accompanied by meaningful commit messages
All new or changed code is covered with unit/integration tests (if applicable)
All automated tests passed successfully (all builds on Travis CI are green)

…eter

orlangur · 2017-08-22T09:09:23Z

Seems overcomplicated to me. Then it's easy to overlook turning this configuration off and it will go to production.

We often find ourselves in two situations

Why each developer does not use own local environment for development?

Probably if we stick to xdebug_session cookie or something it could become something worth merging.

XigenDominic · 2017-08-22T09:33:11Z

I respectfully disagree. When a site is already live and minor frontend updates are being made it makes no sense for each developer to have their own local environment with potentially dummy date. Instead we often use a single client facing shared staging environment with near live product data. Also with the introduction of the parameter code that gives you a bit of protection if the config is left enabled.

korostii · 2017-08-22T10:02:11Z

app/code/Magento/Developer/Model/TemplateEngine/Plugin/DebugHints.php

     */
    public function __construct(
        ScopeConfigInterface $scopeConfig,
        StoreManagerInterface $storeManager,
        DevHelper $devHelper,
        DebugHintsFactory $debugHintsFactory,
-        $debugHintsPath
+        $debugHintsPath,
+        Http $http,


This is a backwards-incompatible change

I understand. Any suggestion on how to make this backwards compatible?

Yes, sure. There's an example in the guides:
http://devdocs.magento.com/guides/v2.0/contributor-guide/backward-compatible-development/#ui-id-3

korostii · 2017-08-22T10:09:55Z

Why each developer does not use own local environment for development?

Even on a local environment it's a burden to go into admin in order every time you want to switch it on or off. Being able to do so using a simple GET parameter is a handy feature to have.

ishakhsuvarov · 2017-08-22T11:09:22Z

app/code/Magento/Developer/Model/TemplateEngine/Plugin/DebugHints.php

+     *
+     * @var string
+     */
+    protected $debugHintsShowWithParameter;


Current Magento Technical Vision does not recommend designing classes for inheritance, therefore please declare properties as private.

*for inheritance

I believe extensibility is still a priority, just achieved with other approaches :)

You are right. Been thinking of extends keyword. Fixed.

ishakhsuvarov · 2017-08-22T11:12:24Z

app/code/Magento/Developer/Model/TemplateEngine/Plugin/DebugHints.php

-                'showBlockHints' => $showBlockHints,
-            ]);
+            $debugHintsParameterInUrl = $this->http->getParam('templatehints');
+            if ((!$debugHintsShowWithParameter) ||


This condition looks hard to read and understand, increasing possibility of an error in future refactoring. Please attempt to simplify it.

orlangur · 2017-08-22T11:29:05Z

@korostii,

go into admin in order every time you want to switch it on or off. Being able to do so using a simple GET parameter is a handy feature to have.

Never tried it via CLI, I'm quite used to Admin UI always opened in separate tab just because you'll need to flush caches together with switch change anyway.

Would be less scary if we'd have Magento_Developer module disabled by design when deployed to production or something like that.

korostii · 2017-08-22T12:17:56Z

I'm quite used to Admin UI always opened in separate tab just because you'll need to flush caches

Oh wow. Yeah, you should definitely try CLI, clearing caches feels much faster this way.

Would be less scary if we'd have Magento_Developer module disabled by design when deployed to production or something like that.

Yes, that makes sense. It's actually quite easy to enforce using composer (by moving relevant modules to require-dev section, see also #8971, #5262 ). Oh, and you'll also need to have the config.php added to .gitignore in order to have different sets of enabled modules between instances.
However there might still be some internal dependencies you'd need to untangle additionally, of course.

piotrekkaminski · 2017-08-22T15:42:39Z

For security reasons we cannot have ability for anyone to just switch on debug info on a site. If such feature is really helpful for developers, it has to be wrapped in IP whitelisting, ie only developers coming from a known, specified IP address can enable it. Then it would not even need the special parameter value, just ?showtemplatehints=1 or similar.

orlangur · 2017-08-22T16:10:15Z

@piotrekkaminski yeah, I was thinking about IP whitelisting as well together with possible filtering by xdebug session. Rejected such option as for scenario provided developers could be sitting in the same office having the same IP.

XigenDominic · 2017-08-22T16:22:52Z

@piotrekkaminski Okay so assuming someone leaves the option enabled and a third party discovers the parameter code and enables template hints is it any more of a risk than say for example someone running your site through magereport.com? The only serious negative I can think is that template hints would get cached and make your site look terrible/broken.

@piotrekkaminski @orlangur I nested my change within isDevAllowed() so surely the option for IP whitelisting is already achieved?

…figurable parameter #10608

Ctucker9233 · 2019-03-17T00:06:34Z

@magento-team Will this be backported to 2.2?

sshymko · 2019-03-20T18:29:49Z

app/code/Magento/Developer/Model/TemplateEngine/Plugin/DebugHints.php

+            }
+            if ($debugHintsWithParam && $debugHintsParameter == $debugHintsParameterInUrl) {
+                $showHints = true;
+            }


Overcomplicated implementation with unnecessarily high cyclomatic complexity.

Equivalent implementation:

$showHints = !$debugHintsWithParam || $debugHintsParameter == $debugHintsParameterInUrl;

I wrote something similar to that at first and the original CI review process told me to change it. But feel free to do whatever needs to be done. I still think this is a useful feature.

Just wanted to leave a comment in case someone stumbles upon it.
I am not part of the Magento team (not anymore) and no action is needed on your part :)

The feature is indeed useful. Thank you so much for contributing it!
Being the original author of that code, I'm just being sensitive to increase of its complexity :)

@sshymko,

Just wanted to leave a comment in case someone stumbles upon it.

How is your comment ever useful to anybody? Please provide a use case.

Thanks for paying attention to code quality though. And also let me inform you that we are accepting pull requests for more than a year already ;) This is the most natural way of constructive collaboration.

I'll try to find time to learn the guidelines and submit the PR.
Until then the comment serves as a reminder to do that.

DominicWatts added 3 commits August 21, 2017 22:23

Option to enable template hints on frontend with a configurable param…

d18887c

…eter

unit test update

f2fcc31

Fixers per failed test

91ba309

ishakhsuvarov added this to the August 2017 milestone Aug 22, 2017

ishakhsuvarov added the develop label Aug 22, 2017

korostii reviewed Aug 22, 2017

View reviewed changes

ishakhsuvarov reviewed Aug 22, 2017

View reviewed changes

backward compatiblity, review changes

d143e5d

ishakhsuvarov self-assigned this Aug 23, 2017

magento-team added Progress: accept develop labels Aug 25, 2017

magento-team merged commit d143e5d into magento:develop Aug 28, 2017

magento-team pushed a commit that referenced this pull request Aug 28, 2017

MAGETWO-71802: Option to enable template hints on frontend with a con…

1fda0a6

…figurable parameter #10608

sshymko reviewed Mar 20, 2019

View reviewed changes

ghost assigned orlangur Mar 20, 2019

orlangur removed their assignment Mar 20, 2019

Option to enable template hints on frontend with a configurable parameter #10608

Option to enable template hints on frontend with a configurable parameter #10608

Uh oh!

Conversation

DominicWatts commented Aug 21, 2017 • edited by ishakhsuvarov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Fixed Issues (if relevant)

Manual testing scenarios

Contribution checklist

Uh oh!

orlangur commented Aug 22, 2017

Uh oh!

XigenDominic commented Aug 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

korostii commented Aug 22, 2017

Uh oh!

ishakhsuvarov Aug 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

orlangur commented Aug 22, 2017

Uh oh!

korostii commented Aug 22, 2017

Uh oh!

piotrekkaminski commented Aug 22, 2017

Uh oh!

orlangur commented Aug 22, 2017

Uh oh!

XigenDominic commented Aug 22, 2017

Uh oh!

Ctucker9233 commented Mar 17, 2019

Uh oh!

sshymko Mar 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DominicWatts commented Aug 21, 2017 •

edited by ishakhsuvarov

Loading

ishakhsuvarov Aug 22, 2017 •

edited

Loading

sshymko Mar 20, 2019 •

edited

Loading