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

Is acquia.inc an appropriate file to include in this repo? #384

Closed
typhonius opened this issue Sep 14, 2016 · 4 comments
Closed

Is acquia.inc an appropriate file to include in this repo? #384

typhonius opened this issue Sep 14, 2016 · 4 comments
Labels
Support A support request

Comments

@typhonius
Copy link
Contributor

Many Acquia customers use this or the D7 version to protect their site/paths. Is BLT a suitable place to have this also included.

As a sidenote to this, it's been raised to me that this could potentially be a D8 Middleware service. Thoughts from smarter people are welcome in this question too.

@dpagini
Copy link
Contributor

dpagini commented Sep 14, 2016

It looks like there's some code in blt/template/factory-hooks/post-settings-php/protect_env.php.examplewhich is supposed to be a ported version of the D7 acquia.inc file (from an Acquia PS project), but this file references a non-existent file in this blt template/project.
I would be interested to see the D8 acquia.inc approach make its way into acquia/blt, and would love to contribute some changes I've made to the script as well.

@grasmash grasmash added the Enhancement A feature or feature request label Sep 14, 2016
@steveworley
Copy link
Contributor

My thoughts are that it should be a middleware service, separate configuration from functionality. Core has the Ban module which allows limited IP blacklisting. It stores IPs in a table and performs queries in the middleware, so I would think checking configuration would pose no problems.

If done as a service it should be a module and could be added via composer during install?

@dpagini
Copy link
Contributor

dpagini commented Sep 15, 2016

I guess I would suggest that if it's an Acquia recommendation for protecting your environment, it makes sense to add it into the acquia (sort of?) framework.

The approach has the file included into your settings.php and configured there, so it could make sense to add it to settings/acquia.inc (I would suggest a different name, but that's a moot point at the moment) in BLT, just to make it available to users who want to use that method. If you dont use it, it's just a 200 line text file that sits in the vendor folder un-referenced.

I don't personally mind too much if it doesn't make it in there, as we can keep it in our repo. I think adding it to the repo gives a great way to open source contribute to the file though.

If I have a chance, I could stage a PR for review & critique at some point, if it's even a road to go down.

@grasmash
Copy link
Contributor

Yeah I agree that it makes more sense as middleware than as part of BLT. Though, I'd be happy to add it as a default dependency for new BLT projects.

dpagini added a commit to dpagini/blt that referenced this issue Sep 19, 2016
Does this make sense here? It's an example file that loads `/sites/default/settings/envprotect.settings.php`, which is a file that does not exist in BLT. Per discussion in acquia#384, the functionality that this file is referencing was decided to make more sense as middleware.
grasmash pushed a commit that referenced this issue Sep 19, 2016
Does this make sense here? It's an example file that loads `/sites/default/settings/envprotect.settings.php`, which is a file that does not exist in BLT. Per discussion in #384, the functionality that this file is referencing was decided to make more sense as middleware.
@grasmash grasmash added invalid Support A support request and removed Enhancement A feature or feature request labels Sep 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Support A support request
Projects
None yet
Development

No branches or pull requests

4 participants