Problem/Motivation

Having the PHP module in Drupal Core encourages new users of Drupal to enable and use it. When people use the PHP module, it can result in:

  1. Security holes when used improperly
  2. Instability when upgrading their site because they may be using some PHP code that might change between upgrades
  3. Performance issues as PHP code in nodes cannot be cached properly
  4. Shared systems, like Drupal Gardens, must hide it from their users in order to ensure safe usage of their service

Proposed resolution

  1. Remove the PHP module from Drupal core
  2. Move it to a contributed module at drupal.org/project/php
  3. People who do want to use it at least have both the access and knowledge to install new modules. If they have this knowledge, it is assumed that they know what they're getting themselves into by using it.
  4. Ensure security updates and improvements are maintained on the contributed module

Remaining tasks

  • Patch needs to be reviewed
  • The contributed PHP module needs additional maintainers

User interface changes

  • The PHP module is no longer in the modules list
  • The contributed module needs to be installed in order to have it back in there

Related

Files: 
CommentFileSizeAuthor
#164 changelog.patch313 bytesRobLoach
PASSED: [[SimpleTest]]: [MySQL] 58,789 pass(es).
[ View ]
#161 1203886-161-do-not-test.patch44.07 KBRobLoach
#160 1203886-156.patch44.07 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 58,766 pass(es).
[ View ]
#157 1203886-157.patch20.05 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1203886-157.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#156 1203886-156.patch44.07 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#152 1203886.patch44.17 KBRobLoach
PASSED: [[SimpleTest]]: [MySQL] 59,141 pass(es).
[ View ]
#150 1203886.patch43.49 KBRobLoach
FAILED: [[SimpleTest]]: [MySQL] 59,213 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#148 1203886.patch19.37 KBRobLoach
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1203886_2.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#138 1203886-2.diff44.24 KBRobLoach
PASSED: [[SimpleTest]]: [MySQL] 58,876 pass(es).
[ View ]
#136 1203886.diff43.65 KBRobLoach
FAILED: [[SimpleTest]]: [MySQL] 57,991 pass(es), 7 fail(s), and 0 exception(s).
[ View ]
#134 drupal 8.x...1203886.diff43.65 KBRobLoach
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal 8.x...1203886.diff. Unable to apply patch. See the log in the details link for more information.
[ View ]
#131 drupal 8.x...1203886.patch18.11 KBRobLoach
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal 8.x...1203886.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#112 1203886_nomore_php_112-do-not-test.patch4.44 KBgreggles
#97 drupal-n1203886-97.patch524 bytesDamienMcKenna
PASSED: [[SimpleTest]]: [MySQL] 58,262 pass(es).
[ View ]
#84 php-1203886-82.patch31.29 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 50,684 pass(es).
[ View ]
#74 1203886.patch31.18 KBRobLoach
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1203886_1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#71 1203886.patch16.56 KBRobLoach
FAILED: [[SimpleTest]]: [MySQL] 36,884 pass(es), 35 fail(s), and 0 exception(s).
[ View ]
#70 1203886-formatpatch.patch17.28 KBRobLoach
FAILED: [[SimpleTest]]: [MySQL] 36,874 pass(es), 38 fail(s), and 0 exception(s).
[ View ]
#44 1203886.patch19.26 KBRobLoach
PASSED: [[SimpleTest]]: [MySQL] 34,096 pass(es).
[ View ]
#37 remove-php-filter-1203886-37.patch19.24 KBDavid_Rothstein
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch remove-php-filter-1203886-37.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
#35 remove-php-filter-1203886-35.patch19.19 KBDavid_Rothstein
FAILED: [[SimpleTest]]: [MySQL] 33,894 pass(es), 20 fail(s), and 0 exception(es).
[ View ]
#33 remove-php-filter-1203886-33.patch18.04 KBDavid_Rothstein
PASSED: [[SimpleTest]]: [MySQL] 33,901 pass(es).
[ View ]
#25 drupal8.php-filter.14.patch14.26 KBcatch
FAILED: [[SimpleTest]]: [MySQL] 32,830 pass(es), 2 fail(s), and 0 exception(es).
[ View ]
#15 bad_judgement.patch26.41 KBcatch
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in modules/simpletest/tests/module_test.module.
[ View ]
#14 drupal8.php-filter.14.patch14.26 KBsun
FAILED: [[SimpleTest]]: [MySQL] 33,133 pass(es), 2 fail(s), and 0 exception(es).
[ View ]
#5 dunwoody.png18.12 KBpillarsdotnet
#4 national_peace_foundation.png71.29 KBpillarsdotnet
php.patch14.41 KBcatch
FAILED: [[SimpleTest]]: [MySQL] 32,048 pass(es), 50 fail(s), and 0 exception(es).
[ View ]

Comments

Why?

Should be in contrib. The namespace is available.

droplet: Because with PHP module in core, people actually use it. And make their sites slow and/or insecure in the process.

There's nothing the PHP module does that can't be done by writing a proper module. All it does is let people cheat and shortcut around doing things the correct - fast and secure - way.

Agreed with pillarsdotnet that it should be a contrib module if anything - with a whole lot of warnings attached to its use.

StatusFileSize
new71.29 KB

Here's a good example.

StatusFileSize
new18.12 KB

Another one.

Status:Needs review» Needs work

The last submitted patch, php.patch, failed testing.

Possible route towards an upgrade path for those who would not like to install the PHP contrib module: http://drupal.org/project/modulate

Tests fail because we're using PHP module as 'a random module' in lots of tests apparently.

Another alternative would be to patch the PHP filter to require that the node author/editor be uid==1 or be a member of the 'administrator' role.

Hm. I'm not sure about this. One really nice thing about PHP module being in core is that as of D7, all modules can use it to hinge their "Use PHP to control X aspect of this module" ... Block visibility, dynamic field values, etc. If it's shut off (and forced off through Paranoia module or similar), all of those other potential holes vanish with it.

Very few modules are going to want to add an external php module dependency, however, so we'll be right back to losing the battle with "Administer FOO PHP PERMISSION" again...

I don't see how a move to contrib affects existing modules all that much, if the PHP stuff is optional, they're using module_exists('php'). If it's not optional, the whole point of the module is putting PHP in textareas and they're specifying dependencies[] = php already - so they don't need to do anything?

Having said that, I'd be fine with a two-step process. Remove the filter from core first - since that's the part that pillarsdotnet demonstrated so amusingly above is not doing anyone any good. Then discuss the general "use PHP for" stuff separately.

+1.

We removed PHP from core when creating a D6 distribution for our university. The reason was because we wanted to deploy thousands of sites, and give site administrators full control of the site, but without allowing them to execute arbitrary PHP code on the server. How much of a real security benefit this was would be hard to quantify, but it was definitely a big step forward in getting Drupal accepted for wide-scale deployment. It also had the added benefit of making it easier to convince people to code actual modules to solve use cases (since they couldn't just throw some random PHP in a block), and thus we could run coder over their code to help them follow best practices.

Title:Remove PHP module from coreRemove PHP text filter from core

Alright I'm re-purposing this a bit.

PHP module currently does two things:

1. Provides drupal_eval() and an associated "Use PHP for settings" permission. This is the part that contrib modules are able to re-use.

2. Provides an text filter that lets you embed PHP in nodes, comments, custom blocks etc. this is the part that allows admins to easily give anonymous users permission to execute PHP on their sites, as so beautifully illustrated above.

While I don't think the former has to live in core, neither is it doing a massive amount of harm there.

However the text filter is actively harmful, and if moved to a contrib module would probably only be used by sites that were upgrading and had lots of legacy PHP content already (and couldn't be bothered to properly convert that into custom blocks). So removing that should be a lot less controversial, and fix most of the issues.

That leaves a PHP module that doesn't actually do anything useful by itself, but sometimes it's better to do nothing :)

Status:Needs work» Needs review
Issue tags:+Framework Initiative
StatusFileSize
new14.26 KB
FAILED: [[SimpleTest]]: [MySQL] 33,133 pass(es), 2 fail(s), and 0 exception(es).
[ View ]

We need to find a maintainer that cares for issues like #486094: Allow to pass local variables to php_eval() -- without resolving issues like this, I somewhat fail to see the actual general purpose usefulness of php_eval() in the first place.

However, we can definitely drop the input filter. Which also solves #826550: PHP module should not create a text format in one shot.

Issue tags:-Framework Initiative
StatusFileSize
new26.41 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in modules/simpletest/tests/module_test.module.
[ View ]

Two weeks later maybe there's a chance.

Status:Needs review» Reviewed & tested by the community

If the bot agrees, i agree as well.

Status:Reviewed & tested by the community» Needs review
Issue tags:+Framework Initiative

For now, I'd limit it to the patch in #14 though - for the reasons @catch outlined in #13.

Status:Needs review» Needs work

The last submitted patch, bad_judgement.patch, failed testing.

I will 'maintain' this module in contrib if it is removed, if at least one person offers to co-maintain it with me.

In maintaining I will do at least the following:

- review patches for 8.x ports

- pointing people to http://drupal.org/project/modulate so they can transition away from it.

- probably add something to shout at people if they enable it.

Sounds like a good idea to me.

I would be willing to co-'maintain' said module with catch.

Note current proposal is to remove the PHP filter, not drupal_eval() and the permission.

Subscribing.

I'm still don't understand....

Security
why #4 ,#5 are security issue. If someone config Drupal into insecure, does it a problem of Drupal? Even writing code in .modules, it can be insecure code too ??

PHP module move to contrib. does it harm own site ?? One good thing of drupal is some guys monitor modules security issue, why we allow a harmful module on d.org ?

Because I'm one using PHP on block on existing sites, really want to know it.
Who can broken your site ?? users with permission or everyone ??

Performance.
It's not an issue of Drupal. every modules & everything can get it slow down, none of your business. (Only enable the modules won't slow down the site ?)

Best practices ..etc
Why you limit users own style?

Can't it make something to improve the PHP filter ??

"pointing people to http://drupal.org/project/modulate"
If this is the good solution & replacement. Can't it get into Core & combine with PHP filter to do some automatically method to get them done.

droplet, removing the filter from core will slow down people who would use it, and maybe give them time to find a better solution in the form of a contrib module which will do what they're looking for, or creating a custom module properly. Those like you who are in the bad habit of using it will still be able to do so with a contrib module if they so choose to do so, but hopefully this will slow down those new to Drupal.

The question is, when you have a problem to solve that requires writing PHP code, what's the proper way to solve it? Is it to use the PHP filter, or to write a proper module? The answer is always to write a module; no exceptions. However, when we have the filter in core, people see it and think that using it isn't that bad; encouraged, even. Removing the filter will avoid that misconception.

Status:Needs work» Needs review
StatusFileSize
new14.26 KB
FAILED: [[SimpleTest]]: [MySQL] 32,830 pass(es), 2 fail(s), and 0 exception(es).
[ View ]

Re-uploading #14 so we can start again from there. Several people in irc did not want drupal_eval or the permission reviewed (including Earl for Views), so one step at a time.

The test failures look dodgy to me.

Status:Needs review» Needs work

The last submitted patch, drupal8.php-filter.14.patch, failed testing.

The module dependency test fail seems like it could be legit, although the test itself looks bunk or out of date, since forum doesn't really rely on PHP does it or perhaps it does in a round about way.

That said, I'd say we should rewrite the module dependency test to enable some other module which has a dependency, but not a dependency on the PHP module. There are plenty of others that we could use in that test.

Feel free to ignore this reasoning if it seems incorrect. Here's the relevant code for that test

Module dependencies (ModuleDependencyTestCase) [Module]
Dependency chain created. Other system.test 467 ModuleDependencyTestCase->testModuleEnableOrder()

448   /**
449    * Tests that module dependencies are enabled in the correct order via the
450    * UI. Dependencies should be enabled before their dependents.
451    */
452   function testModuleEnableOrder() {
453     module_enable(array('module_test'), FALSE);
454     $this->resetAll();
455     $this->assertModules(array('module_test'), TRUE);
456     variable_set('dependency_test', 'dependency');
457     // module_test creates a dependency chain: forum depends on poll, which
458     // depends on php. The correct enable order is, php, poll, forum.
459     $expected_order = array('php', 'poll', 'forum');
460
461     // Enable the modules through the UI, verifying that the dependency chain
462     // is correct.
463     $edit = array();
464     $edit['modules[Core][forum][enable]'] = 'forum';
465     $this->drupalPost('admin/modules', $edit, t('Save configuration'));
466     $this->assertModules(array('forum'), FALSE);
467     $this->assertText(t('You must enable the Poll, PHP filter modules to install Forum.'), t('Dependency chain created.'));

I will 'maintain' this module in contrib if it is removed, if at least one person offers to co-maintain it with me.

I'll help maintain it also. I did the same thing by separating out the Webform PHP module from the Webform project. I maintained it for 1.5 years and then marked it abandoned, but immediately someone else came along to continue supporting it. Removing a module from core is of course a much bigger deal, but something as simple as PHP Filter would be a breeze to maintain in contrib. I'm also behind catch's suggestions to only remove the filter. I'm fine with drupal_eval() and the permission staying in core.

Will a contributing PHP module just add a filter to nodes, comments, etc, and core would still contain the ability to run PHP code in blocks etc?

Aaron, no. If this is done, it will not be done halfway. But there's no reason the contrib module version won't be able to continue to allow PHP code in blocks.

Garret - thanks for that.

Status:Needs work» Needs review
StatusFileSize
new18.04 KB
PASSED: [[SimpleTest]]: [MySQL] 33,901 pass(es).
[ View ]

Well, just to clarify, the current consensus in this issue (and implemented by the patch in #25) would remove the ability to have PHP code in the body of a block, but it does not remove the feature where you can use PHP to set block visibility; that would still remain in core.

Here's a reroll to apply to the latest HEAD, which also:

  • Fixes the broken tests from above. (Re #28, the test there was only failing because it was looking for the string "You must enable the Poll, PHP filter modules to install Forum" but the patch renames the module from "PHP filter" to "PHP". The test itself is OK and not out-of-date; although perhaps for cleanliness it should use its own modules rather than having the test rely on altering the core dependencies, but that's a separate issue.)
  • Fixes up some problems with the module's help text (e.g., outdated references to "PHP filter") and also changes the help text to mention block visibility settings as a specific example where PHP is used, to keep the module's purpose from being too vague and cryptic :)
  • Removed the php.test file entirely (rather than leaving it as an empty file with only a header and no actual tests in it).
  • Removed a couple of other references to the PHP filter in the codebase.

The one remaining reference I found which I did not remove in this patch (and which may need a little discussion) is the reference to the PHP filter in the text_summary() function. For example, this code:

<?php
 
// We check for the presence of the PHP evaluator filter in the current
  // format. If the body contains PHP code, we do not split it up to prevent
  // parse errors.
 
if (isset($format)) {
   
$filters = filter_list_format($format);
    if (isset(
$filters['php_code']) && $filters['php_code']->status && strpos($text, '<?') !== FALSE) {
      return
$text;
    }
  }
?>

It would be great to remove it and just leave it up to the new PHP filter module (in contrib) to alter that behavior back in, but offhand I'm not sure how that would be possible...

@#33 by David_Rothstein on November 20, 2011 at 12:05am :

It would be great to remove it and just leave it up to the new PHP filter module (in contrib) to alter that behavior back in, but offhand I'm not sure how that would be possible...

See #221257-169: text_summary() should output valid HTML and Unicode text, and not count markup characters as part of the text length

StatusFileSize
new19.19 KB
FAILED: [[SimpleTest]]: [MySQL] 33,894 pass(es), 20 fail(s), and 0 exception(es).
[ View ]

Hm, so I actually think it's even simpler than that. I looked into this issue a bit, and it seems to me like the check for the PHP filter here serves no purpose whatsoever. That is because check_markup() has already been called (and therefore, any PHP code has already been evaluated) before the text that's being trimmed is ever passed to text_summary().

That means for our purposes we should just be able to go ahead and remove it. (I've also created an issue at #1347920: text_summary() checks for specific filters, but it should be agnostic to filters for the more far-reaching fix, since there are more things we can remove also.)

The attached reroll therefore removes that last reference to $filters['php_code'] from the codebase. It also removes the PHP module from the list of dependencies for one of the tests I fixed above (since the test is no longer using the PHP filter, it no longer requires the module either).

Status:Needs review» Needs work

The last submitted patch, remove-php-filter-1203886-35.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new19.24 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch remove-php-filter-1203886-37.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

Oops, I got carried away and deleted a little too much from text_summary(). This should work better.

Have I got this right, or am I reading this from somewhere else, because I remember seeing this somewhere, that this new contributing module would remove the separate PHP Filter created by default when enabling this module?

Thanks
AaronMcHale

#37: remove-php-filter-1203886-37.patch queued for re-testing.

Since this is actually a core issue now, I thought I'd name-drop PHP filter. It was a probably-good idea that I ran out of time (and arguably interest) to develop further.

#37: remove-php-filter-1203886-37.patch queued for re-testing.

Issue tags:+Security improvements

I think this issue actually counts as a security improvement, because if the PHP module really were to become nothing more than a wrapper around the php_eval() function and the "use PHP for settings" permission (as proposed here), we can consider making it a really hard requirement that no Drupal module is allowed to execute user-provided PHP code without checking for this module being enabled and this permission being granted.

This will make it easier to isolate PHP execution to one part of the codebase, and for people who don't want to allow it on their servers to more easily block it.

I think this is sort of recommended behavior already, but because turning on the PHP module has these other side effects (creation of the text format and filter, etc) some module maintainers don't really want to tell their users to turn it on in order to unlock "import" functionality or other functionality that requires PHP code to be executed.

Thank you David for summing up one of the reasons why this is an important and needed change. Looking forward to what the test results say here.

Issue tags:+Needs issue summary update
StatusFileSize
new19.26 KB
PASSED: [[SimpleTest]]: [MySQL] 34,096 pass(es).
[ View ]

Attached is a re-roll. Was getting a couple patch warnings. Although I'd opt to removing the module in its entirety and moving it to http://drupal.org/project/php , I'd say this is a good first step. Where in the configuration interface to we ask people to write PHP code? Seems like a major security flaw to me. If Drupal's or PHP's API changes, it could bring down sites.

like how a server's lack of php 5.3 brings down sites? These are deployment issues mostly. I'm all for removing the php filter into a module but I rather like David's take of built in protections against user supplied php in the config.

As for php's use in config, I've mainly seen in present when deciding where a block should be placed or some kind of logic that the module is looking for. A site builder typically gets the option to specify a white list, a black list, or custom php logic.

People new to drupal seem to find the PHP Filter and use it instead of creating custom modules.

It needs to be in contrib with a big warning saying "Are you really sure you want to use this module? why not just write a little custom module, here's the documentation on that."

A typical use case of a CMS newbie: "I want to add the current year in the footer". He searches in Google and found many answers telling about, you just need this Copyright ©

<?php
echo date('Y');
?>

* In wordpress: Just copy/paste that in footer.php of your theme (inside Wordpress admin panel)
* In Drupal, it requires 3 or 4 steps now (enable module, add block, select input filter, etc). If we remove PHP filter from core, add couple of steps more and 5 minutes?

This was just a silly example. I understand the huge security improvement as a Drupal developer and I'm totally in. But we have to be aware that we are making Drupal even more frustrating for not drupal-savy users

EDIT
@pillarsdonet. (you can't edit page.tpl.php from the admin UI, that's what I meant)

In Drupal, edit your page template and add the same text.

Still not convinced we need to store PHP code in the database.

you can't edit page.tpl.php from the admin UI, that's what I meant

So? Use elFinder then.

Yes this module has it's uses, but does it need be in core? No.

I think people grossly underestimate how difficult it is for people new to Drupal when we punt on things with, "Oh, there's a contributed module for that." There are sixteen thousand contributed modules. The chances of finding the one that does what you want when you don't even know what modules are yet are astronomically low. And "Oh, you should really just make a module for your footer text?" Please. :)

I tend to agree that this is basic functionality that I would expect to be included out of the box, and I don't understand why kicking it to contrib is viewed as making anything more secure. It merely makes Drupal core more limited and less useful to various "real world" scenarios that inevitably pop up.

@webchick, have you seen the attachments in post #4 and #5 of this thread? Putting this functionality in contrib is designed to make it harder to do that.

Footer text is entirely customizable in core at the moment, what we'd be dropping support for is putting arbitrary PHP in there which is considerably more obscure.

And if things like putting the current date in the footer are so important it'd be better handled by something like token support.

(Summary of ensuing IRC argument :D)

It only makes it harder to do that because it makes this incredibly useful for newbies functionality impossible to find. Seriously, my first few Drupal sites I couldn't have built without the PHP input format, and I know of several people who eventually found their way to module development through using snippets from the handbook in PHP input format in blocks/nodes. Tokens are nice for things like date, but don't cover numerous other things people might want to put in there.

#4 and #5 simply say to me that we probably need some kind of a security threshold mechanism in core for text formats, similar to our janky permission descriptions but more forceful, that warns the crap out of people if they enable dangerous filters for non-privileged roles. You can pwn a site with Full HTML, too. (Granted, not quite as easily, but enabling Full HTML for anon is way more common than PHP.) Punting this capability to contrib doesn't in any way make that problem go away.

I really strongly feel that if this capability is removed, many new site builders will get completely stuck their first time or two out in building their sites. I'm totally fine with updating the module description to warn people that this is bad juju, but removing it from core altogether I feel would be a huge mistake. We don't babysit people as a general rule; we give them tools they need to solve their problems.

I agree that 'php filter' is a great tool for noobs. It's the way I learnt Drupal, it's the way I still sometimes test code snippets before putting them into a module, but in my day job I see so many huge enterprise level sites that are using it for large sections of the site. It was only a few days ago I saw a site with nearly 1000 nodes with php in. I understand that the education needs to be there to stop people doing this, but as I am often dealing with legacy sites who are just getting around to resolving these sort of issues, it's a little late for them. I am trying to think about new sites and how we can prevent them from making the same mistake.

Moving the PHP filter to contrib doesn't solve the problem of #4 or #5 (only makes it a little harder). After placing it as a contrib module, I'm sure it will be anyway one of the most popular modules.

People will still install the "PHP filter" contrib module and produce anyway all kind of awful sites from Engineers point of view.. that you can't stop it. People, non-coders, want to put custom code everywhere specially 3rd part plugins and stuff like that. Unstoppable.

What can be wiser is to modify the input filter, with something like this in core: #1228748: New security check: PHP filter available to untrusted roles that checks / forbids dangerous configurations (PHP input in comments) and educate Drupal admins of the dangers of PHP as input filter.

------------
I took a little time to research about other CMS:

Joomla: If you order by "popular extensions" the 5th one is "Jumi", that lets you put PHP, Javascript, etc into your content.

Wordpress: core lets you to put php in the templates files via admin interface, but not in widgets and posts. So you need a plugin. One of the most popular ones is http://wordpress.org/extend/plugins/php-code-widget/

Conclusion: Adding custom code is a really popular thing to do, you have it in core or not

PHP filter module is really useful as block filter, damn bad idea as comments input filter (or node input filter)

Maybe is a Drupal chance to rethink the PHP input filter with extra permissions/ checkings, or "pass the potato" to a contribute module as it is. Both solutions have trade offs.

I second timmillwood in #54. We're making it too easy for people to destroy their sites, and practically putting a seal of approval on it by having it in core. I also am perpetually dealing with enterprise sites completely trashed by php in the nodes.

Using PHP in blocks is not quite as bad to me, though, and the use cases for it from noobs are more understandable. Because nodes are meant to be content though, using it there tends to be a part of much worse hacks. Perhaps one compromise would be to change it from a filter to an option for block content only.

We could move PHP Text Filter to contrib, and have it consumed by Spark. Distributions allow us to re-think the way Drupal is packaged, and Spark is a perfect opportunity to use it.

If people use the PHP text filter, it means that when they upgrade to the next version of Drupal, their site will go down. If their host upgrades their version of PHP and they're using an obsolete function, their site will give a fatal error and break. It's a huge security flaw, and ends up being extremely detrimental to new users rather than helping them.

> Seriously, my first few Drupal sites I couldn't have built without the PHP input format
Module Builder is worth mentioning here. webchick, do you think you would have been able to get started integrating new code almost as easily if that helper module was recommended in place of php.module? Both methods aim to provide a shortcut in the module development process, but MB strikes me as a solution that reinforces more sustainable use of Drupal.

Hey, I wrote that module. ;)

And, no, absolutely not.

PHP input format is a matter of: 1) search for problem you're having + "php", 2) copy/paste snippet, 3) save.

Building a module, even with a wizard, is a matter of understanding what a hook is, understanding what hook(s) you need, understanding what files need to go where in your directory tree, and a whole pile of other Drupal-specific things.

And no, moving this to Spark is not an answer, either. That distribution is focused on content authoring experience, so that doesn't make any sense.

So can we just say in big red letters something like (I haven't checked this for pinpoint accuracy):

WARNING: YOU ARE SUBJECTING YOURSELF TO THE FOLLOWING:

  1. Your site will run a bit slower every time you use PHP like this instead of using a module.
  2. If another editor or developer edits this and messes up, it could break some or all pages on your site until it is fixed.
  3. Fixing problems may require you to edit the database.
  4. Pasting PHP you don't understand here may be dangerous and harm your site.
  5. With a modest investment of time, you could use [insert alternative solution here based on where the PHP is being used] instead and face none of these drawbacks.

At least let them know there may be some foot-shooting going on.

Maaaaaaaan I (almost) wish I had the time to work on PHP handler. The project page brings back fond memories. Co-maintainers welcome :D *ahem*

Thought I'd tally up the benefits and cons of having PHP module in Drupal Core:

Benefits of shipping PHP module in Drupal Core

1. webchick at #10
One really nice thing about PHP module being in core is that as of D7, all modules can use it to hinge their "Use PHP to control X aspect of this module"
2. corbacho at #47
Typical use case of a CMS newbie
"I want to add the current year in the footer". He searches in Google and found echo date('Y');. In Drupal, it requires 3 or 4 steps now (enable module, add block, select input filter, etc). If we remove PHP filter from core, add couple of steps more and 5 minutes?
3. webchick at #51
I tend to agree that this is basic functionality that I would expect to be included out of the box, and I don't understand why kicking it to contrib is viewed as making anything more secure. It merely makes Drupal core more limited and less useful to various "real world" scenarios that inevitably pop up.
4. corbacho at #55
Joomla
If you order by "popular extensions" the 5th one is "Jumi", that lets you put PHP, Javascript, etc into your content. Wordpress
core lets you to put php in the templates files via admin interface, but not in widgets and posts. So you need a plugin. One of the most popular ones is http://wordpress.org/extend/plugins/php-code-widget/ Conclusion
Adding custom code is a really popular thing to do, you have it in core or not
5. webchick at #59
PHP input format is a matter of
1) search for problem you're having + "php", 2) copy/paste snippet, 3) save.

Cons of shipping PHP module in Drupal Core

1. Garrett Albright at #3
With PHP module in core, people actually use it. And make their sites slow and/or insecure in the process.
2. coderintherye at #12
We removed PHP from core when creating a D6 distribution for our university. The reason was because we wanted to deploy thousands of sites, and give site administrators full control of the site, but without allowing them to execute arbitrary PHP code on the server.
3. timmmillwood at #46
People new to drupal seem to find the PHP Filter and use it instead of creating custom modules. It needs to be in contrib with a big warning saying "Are you really sure you want to use this module? why not just write a little custom module, here's the documentation on that."
4. catch at #52
If things like putting the current date in the footer are so important it'd be better handled by something like token support.
5. Jody Lynn at #56
We're making it too easy for people to destroy their sites, and practically putting a seal of approval on it by having it in core. I also am perpetually dealing with enterprise sites completely trashed by php in the nodes.
6. Rob Loach at #57
If people use the PHP text filter, it means that when they upgrade to the next version of Drupal, their site will go down. If their host upgrades their version of PHP and they're using an obsolete function, their site will give a fatal error and break. It's a huge security flaw, and ends up being extremely detrimental to new users rather than helping them.
7. webchick at #59
PHP input format is a matter of
1) search for problem you're having + "php", 2) copy/paste snippet, 3) save.

.

@#62 is a big problem but it doesn't work properly ?

http://www.armchairarcade.com/neo/node/1618#comment-26184

EDIT:
im being bad guy and test PHP there, but it prints <?php echo 'abc'; ?> directly without executed

#61 It seems webchick and me are playing the devil's advocate role

Drupal 5 -> Drupal 6 we moved PHP Filter to its own module and to be disabled by default.
If people enables it, it's because they need it, nobody is forcing them.
Placing the module in contrib, only it will rise frustration, but still people will do it.

So...
Could be that the key point here I think is to separate "PHP filter" from to "use PHP for settings" (called "use PHP for block visibility" in D6)

That setting is important, for block visibility and for other contrib modules. For example #870938: "Import view from code" only possible for UID1. merlin wouldn't like to force Views users to enable PHP Input filter for that functionality. That dilemma wouldn't be solve after moving the module "as it is" to contrib space.

Is it an option to split both functionalities and move PHP filter to contrib ?. Also, if we could make that PHP Filter is *always* disabled to use for anonymous role, wouldn't solve #62 ?

@#62 is a big problem but it doesn't work properly ?

It works on some sites, but I'm going to tell you which ones. The scary thing about PHP access is that once you have it, you can manipulate global $user accordingly (not to mention many other scary things).

This is the result of the PHP module being in core. If something is in Core, then people are encouraged to use it. If the module was shipped via a contributed module, then the people installing it at least already have file access to the system. And I assume that means they'd kind of know what they're getting themselves into.

Placing the module in contrib, only it will rise frustration, but still people will do it.

I think potentially having more than 7,130,000 insecure Drupal sites is somewhat more frustrating :-) . You bring up a good point about where do the two places PHP is used go. I'd consider running any PHP code through the browser a blatantly large security hole. We should move it all out to /project/php (to ease the upgrade process). I'm okay with splitting up the functionality too though.

With work going on in #1591686: Add Twig itself and #1499460: [meta] New theme system, we could potentially have "use Twig for settings". Maybe even a Twig Filter too, which would be infinitely more secure than letting users write PHP code in the browser. That'll have to be discussed once that work progresses though.

One really nice thing about PHP module being in core is that as of D7, all modules can use it to hinge their "Use PHP to control X aspect of this module"

That's not relevant to this issue at all, we're only talking about removing the filter here, not drupal_eval() or the permission, so it can be struck off the benefits list IMO.

Exactly, and what we're saying here also is that it's even the other way around (a benefit of the change), since as described above maintainers will be much more likely to require this permission if turning on the core PHP module doesn't have other strange side effects such as creation of a new text format.

But if we can't all agree on actually removing the filter from core, maybe we can at least agree on splitting the module in two (and leaving both halves in core as separate modules) and repurpose this issue to do that for starters? That would still have some of the major security benefits we've discussed here. And also, if we ever do agree to remove it from core, having it already split into a separate module would obviously make that process extremely simple as a followup...

I'd have no objection to a patch that splits the module in two for now.

+1 on splitting in two if nothing else.

Status:Needs review» Needs work
StatusFileSize
new17.28 KB
FAILED: [[SimpleTest]]: [MySQL] 36,874 pass(es), 38 fail(s), and 0 exception(s).
[ View ]

http://drupal.org/project/php ... The "use PHP for settings" permission is moved to system module. We probably need a note in CHANGELOG.txt too.

Issue summary:View changes

Updated issue summary.

Status:Needs work» Needs review
StatusFileSize
new16.56 KB
FAILED: [[SimpleTest]]: [MySQL] 36,884 pass(es), 35 fail(s), and 0 exception(s).
[ View ]

http://drupal.org/project/php ... Updated the issue summary.

Status:Needs review» Needs work

The last submitted patch, 1203886.patch, failed testing.

Looks like the Paranoia module disables PHP module. This still needs a re-roll.

Status:Needs work» Needs review
StatusFileSize
new31.18 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1203886_1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Status:Needs review» Needs work
Issue tags:-Security improvements, -Needs issue summary update, -Platform Initiative

The last submitted patch, 1203886.patch, failed testing.

Status:Needs work» Needs review

#74: 1203886.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Security improvements, +Needs issue summary update, +Platform Initiative

The last submitted patch, 1203886.patch, failed testing.

I heavily support this, every day I have calls with customer pleading them to disable the PHP filter module listing security and performance impacts.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

dsffds

I hadn't heard of the performance problems with PHP filter.

I definitely agree with it being yanked from Core though. Just too easy to slip into bad practices and have more PHP in the database rather than in a repository.

This patch applies nicely locally.

Status:Needs work» Needs review

It'd still be great to get this into D8.

On a daily basis I am advising people to rework their sites not to use PHP Filter, and it seems to be assumed it's ok because it's in core.

Someone has to re-roll the patch with updated tests in it unfortunately. Agreed that it would be important to put in to D8 if we can!

Requires a re-roll... On the plus side, new users can post PHP. On the negative side, new users can post PHP.

In general, I believe that the PHP filter supports bad practices and encourages new users to adopt these poor behaviors. This leads to a high frequency of unstable and insecure sites each one of which can lead to a negative perception of the greater project. Easing a new users approach to implementing custom PHP should be achieved via alternative paths and is important, but the use of the PHP Filter to support the ease of introduction to the Drupal project is "penny wise and pound foolish".

Common related issue:

Discussed in greater detail and a mention of poorly written PHP code as breaking cron:

StatusFileSize
new31.29 KB
PASSED: [[SimpleTest]]: [MySQL] 50,684 pass(es).
[ View ]

Attempted reroll.

Status:Needs review» Reviewed & tested by the community

Updated to the latest in the PHP module.

Assigned:Unassigned» Dries

This is another one of these.

Status:Reviewed & tested by the community» Needs work
  1. With this patch applied there are still calls to module_exists('php') and php_eval() left in the codebase, despite the module and function being removed (guess the test coverage is lacking)... There is also Views which calls eval() directly, but that's partially a separate issue.
  2. diff --git a/core/modules/system/system.module b/core/modules/system/system.module
    index cdf7b90..2d21dad 100644
    --- a/core/modules/system/system.module
    +++ b/core/modules/system/system.module
    ....
    +    'use PHP for settings' => array(
    +      'title' => t('Use PHP for settings'),
    +      'restrict access' => TRUE,
    +    ),

    Having that in system.module looks wrong on so many levels.

Also not clear where this issue is going:

  1. From #13 onward this issue has only been about removing the PHP text filter from core, not the whole module. The issue title still reflects that.
  2. In #67, #68, and #69 there was even some agreement on merely splitting the module in two for now (without removing anything), since that would be a required first step anyway, and it would also be uncontroversial and have huge security benefits in and of itself.

The latest patches, however, remove the whole module... Why did the issue change gears again?

There is also Views which calls eval() directly, but that's partially a separate issue.

Let's open an issue for that: #1889718: Drop php argument validation and default argument

Assigned:Dries» Unassigned
Status:Needs work» Closed (won't fix)

I'm not actually convinced we want to remove this module from core. I'd prefer to keep it in core.

Title:Remove PHP text filter from coreSplit the PHP text filter out of the main PHP module (and consider removing it from core later)
Status:Closed (won't fix)» Needs work

See #88.......

Title:Split the PHP text filter out of the main PHP module (and consider removing it from core later)Split the PHP text filter out of the main PHP module (and consider removing the filter from core later)

Rewording the title to be even more explicit...

Priority:Normal» Major

I say it dies.

+1. Seriously, code does not belong in the database. If you want to write code, write it in a module. IMO, the optimal solution is this:

git rm -rf core/modules/php

but I'd settle for just removing the php text filter.

It's such a powerful tool. We've inherited so many sites where someone has just threw in PHP willy-nilly into a node. It's like a spoke wrench. In the right hands it will make your wheel true, but for the vast majority your more likely to break a spoke or more.

It would just be nice to make it not so accessible. If folks had to install a module before being able to access the filter they might think twice and just writing the PHP properly in code rather than taking the obvious short cut and adding it to the node/view.

No. The first problem is that people keep calling this a tool. Code in the database is a bug. Period.

This is easily solved by a bit of documentation:

Rather than putting code in your node body, copy this function into a new module (then provide an example hook_node_view implementation). Rather than putting code in your block body, copy this function into a new module (then provide example hook_block_view implementation). This documentation page would take 30 minutes to write, and if that's all that's needed to remove php module from core, I'll volunteer to write it. This problem is easily solved with documentation, and we should not make it easy for code to be put back into the database.

StatusFileSize
new524 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,262 pass(es).
[ View ]

I'd also recommend adding a warning to the description of the PHP module to indicate it is a security hazard.

Status:Needs work» Needs review

#96: Perhaps a new tool could build on the possibility to write PHP files to appropriately solve the use case of coding within the site builder UI.

Here's how to "to appropriately solve the use case of coding within the site builder UI": don't do it. I will never support including that kind of tool in core, and I will never work on one in contrib. If you want to write code, do it the right way: in a code editor.

Mitchell, also, chx already wrote that: https://drupal.org/project/modulate

Status:Needs review» Needs work

Likely need an update!

Ran into the PHP module last week. Tried to update a Drupal site, and it resulted in a #whitescreenofdeath. The reason was because a node was using the PHP filter, and calling part of the Twitter module to show recent Tweets. Unfortunately, the Twitter module's API has recently changed, so updating the Twitter module made the PHP filter code fail, and caused a white screen of death. You could blame the Twitter module, but no, it's people thinking using the PHP module is a good idea because it's part of Drupal core.

Let me reiterate... This module sucks and should be avoided. Please don't let it live in Drupal core. We should promote good practices, not terrible ones that cause your site to die.

+1 RobLoach

+1 RobLoach

The Ubuntu forum attack and downtime post mortem.

Once the attacker gained administrator access in the Forums they were able to add a hook through the administrator control panel. Hooks in vBulletin are arbitrary PHP code which can be made to run on every page load. The attacker installed a hook allowing them to execute arbitrary PHP passed in a query string argument. They used this mechanism to explore the environment and also to upload and install two widely available PHP shell kits. The attacker used these shell kits to upload and run some custom PHP code to dump the ‘user’ table to a file on disk which they then downloaded.

Get PHP filter TF out of core, please.

Assigned:Unassigned» Dries

Back to Dries. #90 was more than six months ago and doesn't have any justification for marking won't fix.

Without #94 (or paranoia or modulate modules), anyone with sufficient access to a Drupal site can enable the php module and execute arbitrary php code. Of course, it is very bad if someone happens to obtain or guess an admin password on a site, but arbitrary code execution makes this so much worse.

Not only should php module move to contrib, it should also require Bad judgement, as shown by #102, #5, #4, etc. Let's get this out of core.

Just throwing my belated voice in here: Yes, we do want to remove PHP module from core. I want to remove it from contrib, too, but one battle at a time. :-) I'm with Rob and cweagans. PHP code in the database is a bug, not a feature.

php.module--

@greg.1.anderson that's already implemented to an extent: #786208: Support PHP module.

@catch well done. Now we only need to decide whether we move php module out of core, or bad judgement in. :)

Guess it's now up to Dries...

StatusFileSize
new4.44 KB

I also read the Ubuntuforums post-mortem and thought of this issue.

I also would like to see the module removed from core by leaving the permission.

To say that it needs to be in core to ease the onramp ignores the fact that Drupal's users are comfortable downloading modules.

Attached is a patch that I think gets part way there, though tests need work.

Another option would be to bring in something like the Paranoia module in Core. It took a while for that module to get upgraded to D7.

Issue tags:+Security improvements

I assume at least this tag removal was unintentional.

I don't see paranoia module in core as a good answer. It actually removes the "use php for settings" permission which I think needs to be in core (so it can be used by contrib, so that paranoia can block it). It's also a much more extreme solution than the proposal from comment #13 on this issue so if #13 is controversial then paranoia is more controversial.

Issue tags:+Platform Initiative

Yes. No idea why it was gone again when I started editing this reply. Added back in the Platform Initiative too which I had removed (without doing anything intentionally or randomly hitting the keyboard).

Mind you I'm in Rangoon at the moment. The Internet isn't so reliable here.

Assigned:Dries» Unassigned

Thought about it more and I think it makes sense to delete this module in its entirely, and offer it as a contributed module (either in full or partially).

I'm hopeful you mean "remove everything except the permission" and not "remove it entirely". We're slowly coalescing around contrib modules using this permission to control access to executing php and it would be a shame to lose momentum on that front.

The permission could probably just move into system module.

I have a vague memory of the permission being in the module so that it was easier for paranoia etc. to prevent people ever getting the permission, but I'm not sure if there's any use cases that couldn't be solved if it's just in system module too.

Devils advocate: what's the purpose of retaining a permission for which core has no use? Is there precedent for this?

It doesn't matter much to me whether the permission is in php.module or system.module.

Paranoia blocks enabling php.module and it blocks granting the "use PHP for settings" permission to any roles.

By keeping the permission in php.module, someone could get some of the protections of paranoia just be removing the php.module from their site. If we move it to system that idea goes away. So, I guess I have a slight preference for leaving it in php.module out of respect to people who like deleting php.module from their site.

Regardless of where it is, uid 1 can still bypass it (unless #540008: Remove the special behavior of uid #1. gets in).

Issue summary:View changes

fdsa

@DamienMcKenna - I've updated the issue summary to give some ideas. If you need more basis search the issue comments for the word "permission."

The purpose of the permission being added in D7 is to provide a single "killswitch" for removing PHP access from all users (except UID1 per #121). If this permission isn't provided by core, then every contrib module providing this functionality starts doing separate "Use PHP for Block Visibility", "Use PHP for Views", "Use PHP for Ponies" permissions (as it was in D6) which are much harder to audit.

Oops. Cross-posted with an issue summary update. :D Thanks, greggles!

I have no objection to retaining the permission in core, either in system.module, or in php.module. However:

If this permission isn't provided by core, then every contrib module providing this functionality starts doing separate "Use PHP for Block Visibility", "Use PHP for Views", "Use PHP for Ponies" permissions (as it was in D6) which are much harder to audit.

Is that the only option? Why can't there be a canonical php.module in contrib that offers the permission and the eval wrapper, and all other contrib modules use that?

True enough, I guess. But since one of the arguments against this feature was "if it's in core, people will use it," it seems advantageous to keep such a killswitch in core so that people will use it. :)

Just to seek clarification again, what is the value of considering something (a permission) a global killswitch when a) user 1 is unaffected, b) most of the dangerous, security-threatening, worst-case-used-of-a-technology-ever happens through user 1 anyway?

This was acknowledged in comment #121 with the reference to #540008: Remove the special behavior of uid #1..

Element b of the hypothetical in #127 hypothetical is false.

Edited to make it clear what hypothetical I was referring to

Just to mention it, the only times I've seen the PHP module used it was for user 1, hence my reservations.

Resolving #540008: Remove the special behavior of uid #1. would relieve most of my concerns, and I'm sorry for overlooking its gravity when it was first mentioned.

Back to lurking :)

Status:Needs work» Needs review
StatusFileSize
new18.11 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal 8.x...1203886.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Status:Needs review» Needs work

The last submitted patch, drupal 8.x...1203886.patch, failed testing.

Status:Needs work» Needs review

You're not removing the actual filter (\Drupal\php\Plugin\Filter\Php)

StatusFileSize
new43.65 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal 8.x...1203886.diff. Unable to apply patch. See the log in the details link for more information.
[ View ]

Status:Needs review» Needs work

The last submitted patch, drupal 8.x...1203886.diff, failed testing.

Status:Needs work» Needs review
StatusFileSize
new43.65 KB
FAILED: [[SimpleTest]]: [MySQL] 57,991 pass(es), 7 fail(s), and 0 exception(s).
[ View ]

Used a seperate remote as the mirror wasn't quite up to date:
https://github.com/RobLoach/drupalmirror/compare/1203886

Status:Needs review» Needs work

The last submitted patch, 1203886.diff, failed testing.

Status:Needs work» Needs review
StatusFileSize
new44.24 KB
PASSED: [[SimpleTest]]: [MySQL] 58,876 pass(es).
[ View ]
  1. Rebased to a new branch
  2. Security test fix: Interdiff

#138: 1203886-2.diff queued for re-testing.

Issue summary:View changes

updated to focus on the current proposal as put forth and endorsed by catch, David_Rothstein, etc.

Title:Split the PHP text filter out of the main PHP module (and consider removing the filter from core later)Remove the PHP module from Drupal core

Updated the issue summary, and ensured the PHP module is functional.

Issue summary:View changes

fff

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

You updated the issue summary, so we can remove that tag. :)

Status:Needs review» Reviewed & tested by the community

I don't see the PHP module in the list of available modules anymore. I don't see options to write custom PHP code in the block visibility settings. I don't see a PHP input filter option when editing text formats.

The tests pass!

The code looks good!

I'd say this is RTBC!

Title:Remove the PHP module from Drupal coreRemove the PHP module from Drupal core, and split the text filter and Views integration out of the main module

I'm not changing the status from RTBC because I think the module could be removed first and these other issues dealt with later, but are the concerns raised by @greggles, @webchick, and others answered? And I think we still need to make sure the module is split up at some point; simply dumping it in contrib as is won't provide the security benefits.

To clarify that, the only "safe" way for a module to execute user-provided PHP code (in such a way that even user 1 cannot directly bypass it) is to:

  • Check module_exists('php') && user_access('use PHP for settings') before allowing the code to be entered.
  • Check module_exists('php') before allowing the code to be executed.

This is best practice currently, but I think we should aim to enforce it (at the security team level) in Drupal 8. The path to that is:

  1. In Drupal 7, module maintainers have intentionally (and justifiably, I think) resisted the module_exists('php') part of the above, because they don't want to force their users to endure the unwanted side effects (the extra text format, etc) of turning that module on (see #870938: "Import view from code" only possible for UID1 for the Ctools issue around this). In Drupal 8, last time I checked it's even worse because some Views code had moved into the module too; the module has gotten bigger when it should be getting smaller. To force module maintainers who want to execute PHP to rely on the PHP module, we have to strip it down and split all the side effects and features out of the main module.
  2. Although the above two code checks aren't that complicated, they are just subtle enough that I think people will manage to get them wrong by accident too. So there could be some benefit to having easy-to-remember API functions as wrappers around the above, something like php_can_be_entered() and php_can_be_executed(). By definition, these API functions would have to live in core. However, I don't think that necessarily means anything else would need to live in core; although it would be unusual for code in Drupal core to check for the existence of a module and permission that live in contrib, it's not completely unprecedented for core code to assume something about contrib modules (at one point I believe there were a few things in core that were specific to the Devel module). I think this is one of the rare cases where it would make sense. (And for extra fun, we could even consider making core use these functions in #932110: On some servers, the Update Manager allows administrators to directly execute arbitrary code even without the PHP module, but that's its whole own discussion.)

Does that sound like a path forward to address the concerns above?

Assigned:Unassigned» Dries

I'd be OK with adding two helpers like that to core. Assigning this to Dries, I opened this issue in 2011 so it's clear what my view is.

Status:Reviewed & tested by the community» Needs work

Per #143, I think we can deal with these issues later and commit this now. Patch needs to be re-rolled though.

Issue tags:+Needs followup

Tagging for a follow-up issue in #143.

Small suggestion: the patch will reroll more easily and be smaller to review with git config --global diff.renames copies.

Status:Needs work» Needs review
StatusFileSize
new19.37 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1203886_2.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
  1. Rebased and merged the conflict
  2. @xjm There aren't any renames in this diff so that does not affect the size of the patch. You might have meant to suggest -D however, which will only print the header of the files rather than the diff to nothing. I did that in this patch for ya.

Status:Needs review» Needs work

The last submitted patch, 1203886.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new43.49 KB
FAILED: [[SimpleTest]]: [MySQL] 59,213 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

It appears that results in a diff that applying seems to not enjoy. This is it without -D. For those following at home:
https://github.com/RobLoach/drupalmirror/pull/1
https://github.com/RobLoach/drupalmirror/pull/1.diff

Status:Needs review» Needs work

The last submitted patch, 1203886.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new44.17 KB
PASSED: [[SimpleTest]]: [MySQL] 59,141 pass(es).
[ View ]

Title:Remove the PHP module from Drupal core, and split the text filter and Views integration out of the main moduleRemove the PHP module from Drupal core

Just changing the title back. Looks like it got reset accidentally.

(Also... zomg +1)

The title change was on purpose, but it seems like we've agreed to deal with that in followups.

Hopefully the maintainer of https://drupal.org/project/php will agree with a "Remove most of the PHP module from the PHP module" issue, though :)

Status:Needs review» Reviewed & tested by the community

Since that looks like it was a straight-up re-roll, moving back to RTBC.

StatusFileSize
new44.07 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Patch no longer applied. :( core/modules/php/lib/Drupal/php/Tests/PhpFilterTest.php had changed in #2083811: Remove langcode nesting for fields in entity $form structures and this patch deletes it

StatusFileSize
new20.05 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1203886-157.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Actually let me upload the nice to review patch generated with git diff -D 8.x HEAD

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 1203886-157.patch, failed testing.

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new44.07 KB
PASSED: [[SimpleTest]]: [MySQL] 58,766 pass(es).
[ View ]

Well who know -D is only for reviewing should have read more of the comments :D uploading the patch from #156 again

StatusFileSize
new44.07 KB

Rebased and it resulted in the same was what Alex posted. +1

Status:Reviewed & tested by the community» Fixed

Committed to 8.x. Live from NYC. Finally. Let their be rejoicing. :)

Hooray!

Title:Remove the PHP module from Drupal coreFollow up: Remove the PHP module from Drupal core
Status:Fixed» Needs review
Issue tags:+CHANGELOG.txt, +Needs change record
StatusFileSize
new313 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,789 pass(es).
[ View ]

Three things:

  1. Change notification needs review: PHP Filter module removed from Drupal core
  2. Attached patch for CHANGELOG.txt needs a review
  3. How do we proceed with the points brought up in #143?

Status:Needs review» Reviewed & tested by the community
Issue tags:-CHANGELOG.txt, -Needs change record, -Needs followup

Patch only removes the php filter from CHANGELOG.txt.

Opened follow up issue #2088851: Handle remaining security/permissions issues related to removing the php module from core.

Issue tags:+CHANGELOG.txt

Oops

Title:Follow up: Remove the PHP module from Drupal coreRemove the PHP module from Drupal core
Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks!

This is nonsense.

We cannot have this in the core because "Having the PHP module in Drupal Core encourages new users of Drupal to enable and use it ..."

This is like saying lets stop making cars because they encourage new and inexperienced drivers to use it and cause accidents.

Come on, please ...

@ami7878: I don't know how many Drupal websites you've worked on, but the experiences of probably everyone else in this issue is that the PHP module, specifically the PHP text filter, is used by people who shouldn't have used it in the first place, who leave massive security holes in their sites through their inexperience. IMHO I'd go so far as to say that Drupal never should have had the PHP module in core in the first place, but I'm happy it's finally gone.

Lets be clear about something: solving a problem by using PHP code in a custom block or a node form field is always the wrong way of doing it.

Lets move on.

New drivers must go through training and licensing programs to make sure that they know how to operate a car properly.

This is more like, instead of shipping cars with a stereo system that, when turned on, disable the airbags and anti-lock brakes and turn the car's body into a high-powered magnet, let's stop shipping cars with a stereo system and instead make the user install their own stereo system with those effects if they wish.

I'd go so far as to say that Drupal never should have had the PHP module in core in the first place

Me too. I've felt that way pretty much ever since I discovered this module existed.

PHP module is more like selling a car with a small fridge in front of the driver's seat filled with 12 cans of beer.

Automatically closed -- issue fixed for 2 weeks with no activity.

Issue summary:View changes

Updated issue summary.