Skip to content
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

Extended activation configuration / depend for PHPStorm URL #21

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

amenk
Copy link
Contributor

@amenk amenk commented May 24, 2013

No description provided.

@fbrnc
Copy link
Member

fbrnc commented Jun 12, 2013

Hi,

thanks again for another contribution.

First of all a small optimization: Let's check the store parameter at the end of the if() as reading from the request object is faster.

Now the problem I'm running into: When having the module set to "always on" and to "module enabled" I cannot access the backend anymore. I'm not sure if this is a problem related to my dev environment or if this is a generic solution. Can you confirm?

In addition to that I don't know if we should have another option to have the module enabled or not. I guess it makes sense with the always on feature. But it might be confusion for user that are upgrading or installing it without checking the options. We might enable it by default. The protection should be done through dev mode anyways.

@amenk
Copy link
Contributor Author

amenk commented Jun 12, 2013

First:

What is the problem accessing the admin interface? I have checked it (actually in a Magento 1.6.2) and it is basically working. But the layout is a little bit different - what kind of problem are you running into?

Second:

Basically there is a general problem in Magento with the dev mode, as the IP filter is blank by default.
But at least the template hints in Magento are off by default.
Now the problem with your template hints is that they are "invisible" by default - I find this risky because somebody could install them and then forget them while it is still possible to enabled them using ?ath=1 ...

Of course it is generall question if that is the responsibility of the module or if the users should just think about that themselves ... on the other hand security-by-default is not a bad thing.

If we leave module enabled on, I think we should have always on also on, so at least there is something visible after installing the module.

To conclude I do not find it a big regression if the module is disabled by default and you are forced to enabled it in the backend. -> we could write a note in the readme.

Better the users are forced to read & enable it than having it accidentally on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants