As its README states, devel is designed to be safe for Production use, should its operator desire that and be capable enough to understand the risks.
In this maintainer's opinion, and in the opinion of the Drupal Security Team, devel's Execute PHP form is a bit too risky, even for knowledgeable users. As such, we plan to remove this feature. Users who miss it are encouraged to use and improve the PHP module.
As its README states, devel is designed to be safe for Production use
Finally, while the README does state that, it seems kind of weird to me. I have rarely seen a need to run it in production, and indeed almost all Composer-based builds nowadays add it as a dev dependency (so it's not even available to install on production)... Not quite sure what the value would be of encouraging it to be available in a production codebase.
But that's neither here nor there, since that's my subjective opinion (namely, "development tools shouldn't exist in production")—my main sticking point is this is a major feature removal in the 1.x branch, and even though Drupal.org modules don't follow strict semantic versioning, anyone updating the module in the 1.x branch who uses this handy tool will get a nasty surprise.
Comment | File | Size | Author |
---|---|---|---|
#14 | devel-remove-execute-php-3005475-14-8.patch | 5.62 KB | lussoluca |
| |||
#5 | devel-3005475-5.patch | 631 bytes | mikhailkrainiuk |
|
Comments
Comment #2
moshe weitzman CreditAttribution: moshe weitzman as a volunteer commentedComment #3
kala4ekWhy does anyone need to use the Devel module at PRODUCTION?
Devel does not need at production at all.
If this issue will be done, so... it's fork time :)
Glad that anyone can publish a new module, like Better Devel
Comment #4
mikhailkrainiuk CreditAttribution: mikhailkrainiuk as a volunteer and at DrupalJedi commentedHello!
Yes, this module is nice to development. But we disable it on production websites in most cases.
I created the patch to README to fix it :)
Verify it, please.
Thank you!
Comment #5
mikhailkrainiuk CreditAttribution: mikhailkrainiuk as a volunteer and at DrupalJedi commentedComment #6
kala4ek+1 Really useful patch.
Comment #7
lussolucaMaybe we can move the Execute PHP feature to a new Devel's submodule?
In this way users can install Devel in production without any risk and we can put a security alert only if Execute PHP submodule is enabled.
Comment #8
moshe weitzman CreditAttribution: moshe weitzman as a volunteer commentedThe README patch is rejected.
Moving Execute PHP to a submodule helps minimize the risk, but it doesn't minimize enough. I want this project to keep its Security Team coverage, and we can't do so with this feature, even in a submodule.
Please consider using and improving PHP module (or make your own). Thats the module that carried on Drupal core's arbitrary PHP runner, once that feature was removed for security reasons in D8.
Comment #9
salvisLink newly closed Execute PHP issues that may want to come along to a new home...
@moshe weitzman: What's the plan for D7?
Comment #10
lussolucaThis patch removes the execute php feature from Devel.
Meanwhile I'm committing a new module: devel_php that re-adds this functionality outside of Devel
Comment #12
lussolucaComment #14
lussolucaComment #15
lussolucaTest are passing and here there is the new module: https://www.drupal.org/project/devel_php
Comment #17
lussolucaCommitted to 8.x-1.x
Comment #18
willzyx CreditAttribution: willzyx commented@luca @moshe while I totally agree with the IS and the php execution code removal, I think that this changes, since are a BC break, should not be committed in the 8.1.x branch and released in a minor/patch version.
There are modules that rely on this feature and a simple update to the next minor devel release is going to break their functionalities
https://www.drupal.org/project/devel_ace
https://www.drupal.org/project/devel_clipboard
https://www.drupal.org/project/devel_snippet
https://www.drupal.org/project/devel_codemirror
https://www.drupal.org/project/admin_toolbar (#3009193: Error because devel module has removed the execute php feature)
...
(I made a very quick search but I suspect that the number of modules is definitely greater)
IMO this commit should be reverted for 8.1.x and should be handled in a 8.2.x branch
Update:
Add devel_codemirror, admin_toolbar
Comment #19
salvis@willzyx: None of the three modules that you mention has a release version, and all three report only single-digit usage counts. The effort of branching far outweighs the inconvenience of having to update those modules IMO.
Comment #20
willzyx CreditAttribution: willzyx commented@salvis
This changes very little in what I'm saying. The reason for which we use a simil-semver model here and in drupal core is exactly for avoid this disruptive situations.
Also note that: I made a very quick search but I suspect that the number of modules, that implicitly or explicitly, rely on this feature is definitely greater
IMHO create a new branch, even is more costly in term of effort, is the right way to go.
(And we also consider that in the issues queue there is more than an issue that is waiting for a new branch for being commited)
Comment #21
joachim CreditAttribution: joachim as a volunteer commented> I think that this changes, since are a BC break, should not be committed in the 8.1.x branch and released in a minor/patch version.
This change has broken the Admin Toolbar module, which expects to find the exec PHP route.
Comment #22
bojanz CreditAttribution: bojanz at Centarro commentedSo, one of Devel's main features will be removed in a point release?
Can't say I find that to be wise.
Comment #23
czigor CreditAttribution: czigor at Centarro commenteddevel/php is pretty much 95% of what I use this module for. Why did it suddenly become a problem?
Comment #24
klausiAgreed on the BC break. Please create a 2.x branch for devel and revert this change on the 1.x branch. Then mark the 1.x branch unsupported because the security team does not allow support.
Comment #25
rp7 CreditAttribution: rp7 for Government of Flanders commentedSince this functionality is part of my "toolchain", I'm currently (until this hopefully gets fixed) adding devel to my projects as such:
composer require drupal/devel:1.x-dev#86e3b8f40c6fc8a452098b975bc9d88eeb56d774
Comment #26
Chris CharltonThank you, @rp7 (#25) for the suggested composer command that lets us update all-but-up-to-that-one-removal-of-exec-php. :D
Comment #27
geerlingguy CreditAttribution: geerlingguy at Acquia commentedI kind of have to agree with:
I quite often drop a few lines of code from a module I'm testing into a plain local D8 site's devel/php textbox to make sure they function without fully testing the code yet...
I had no idea that (which is probably one of the 3 features I use 99% of the time I use the Devel module at all) would be considered insignificant enough to be dropped in a non-major release.
I would strongly advise doing that in a 2.x, and leaving 1.x with 'unsupported' so people know they should upgrade, making sure the https://www.drupal.org/project/devel_php is mentioned in the release notes.
Comment #28
geerlingguy CreditAttribution: geerlingguy at Acquia commented(Plus it breaks Admin Toolbar locally, which I use on pretty much all my sites.)
Fixing the issue summary, oops!
Comment #29
drunken monkey+1 for keeping the feature in an (unsupported) 1.x branch and only removing in a new 2.x branch.
Breaking Admin Toolbar should really be enough of a reason (certainly doesn’t fall under “single-digit usage counts”).
Comment #30
Chris CharltonThe 1.x vs 2.x split is logical, and a common pattern in Drupal-land. There would be no confusion having that be documented at the top of the 2.x release notes & project page summary.
Comment #32
lussolucaSorry for the delay.
I've reverted the commit on 8.x-1.x branch and created a new 8.x-2.x branch.
Now @moshe, we can release 8.x-1.3 (and mark it as insecure) and 8.x-2.0 that doesn't contain the PHP execution feature.
Comment #33
lussolucaComment #34
temkin CreditAttribution: temkin at SiteCraft commentedWould be helpful to add a note in 2.0 release notes that it breaks Admin toolbar module. It's one of the most popular modules out there, so Devel upgrade to version 2.0 will break a lot of sites.
Comment #35
moshe weitzman CreditAttribution: moshe weitzman as a volunteer commentedI think it breaks older vesions of Admin Toolbar, not the current one. We could put in a conflict statement in composer.json and also a statement in the info.yml to prevent problems.