Remove Execute PHP feature

Why does anyone need to use the Devel module at PRODUCTION?

devel is designed to be safe for Production use

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

Hello!

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!

+1 Really useful patch.

Maybe 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.

Link newly closed Execute PHP issues that may want to come along to a new home...

@moshe weitzman: What's the plan for D7?

This 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

Test are passing and here there is the new module: https://www.drupal.org/project/devel_php

Committed to 8.x-1.x

@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

@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.

@salvis

None of the three modules that you mention has a release version, and all three report only single-digit usage counts.

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)

> 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.

So, one of Devel's main features will be removed in a point release?
Can't say I find that to be wise.

devel/php is pretty much 95% of what I use this module for. Why did it suddenly become a problem?

Agreed 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.

Since 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

Thank you, @rp7 (#25) for the suggested composer command that lets us update all-but-up-to-that-one-removal-of-exec-php. :D

I kind of have to agree with:

So, one of Devel's main features will be removed in a point release?
Can't say I find that to be wise.

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.

(Plus it breaks Admin Toolbar locally, which I use on pretty much all my sites.)

Fixing the issue summary, oops!

+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”).

The 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.

Sorry 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.

Would 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.