[meta] Add .php extension to PHP files

That's why we lock the directory in .htaccess. Any reason why this not sufficient?

yes - that solution is specific to apache web server.

.inc.php file "protection" is also server configurable.

If the web server adminstrator screws up his configuration, some files which should be hidden will not be.

If the web server administrator knows what they are doing, it doesn't matter what the files are called. They will be safe.

Defaulting towards Apache is not a bad idea. Apache runs more web servers than all the other programs combined, currently at about 67% of all servers.

+1 for renaming all *.inc to *.inc.php. and all *.(module|theme) to *.(module|theme).php. beside being more secure and shrinking the .htaccess, this makes drupal more "standard" and "out-of-the-box" accessible for development (code editor file associations, debuggers, etc.) and other purposes. doxygen, for example, doesn't work with the current naming scheme out-of-the box but has to be patched to recognize *.(module|theme). quite hard under windows ...

+1 to ax's suggestion

It's a reasonable suggestion, but it would take major re-architecting, and need not affect users with proper server configurations, so I'm reclassifying this as a feature request for HEAD (making setup easier in hosted environments) rather than a bug report for 4.4.

This is a very interesting idea, but I don't see this as a threat.

All of the main settings and usernames/passwords are stored in settings.php ... no way that is going to get through. Also, phpTemplate is the default now and those files end *.php as well. Only files that don't are modules and inc files but there is nothing sensitive in those pages.

Marking those closed as it really isn't needed anymore.

Re-opening this. Having consistent naming of .php files would:

1. Make it easier to see what's php and what isn't when visually scanning a folder.
2. Save having to configure text editors to recognise those files as PHP
3. Help people on crappy hosting lock those files down a bit more.

Even if people think 3 is unreasonable, 1 and 2 are pretty good reasons IMO.

Actually I think it easier to scan a directory when all the files don't end in .php, .module, .info, .theme, even .inc tell me a lot about the file with out looking at it.

I am advocating this for quite some time now :) (I know I know moving stuff out of webroot is best, but many hosts won't allow for it).

@nevets - we could append .php (I think that's catch's suggestion) - so, for example, node.module.php, node.pages.inc.php

Decreases readability for me and there is an advantage to not using .php since by default those files are not executable by the PHP interpreter

subscribing. I'll patch this if it would be accepted. what are the issues with this breaking all the patches in the system though? is there a best time to do it?

Major changes to do with moving files around are happening at the moment (eg #22336: Move all core Drupal files under a /core folder to improve usability and upgrades ) so maybe it makes sense to do this now-ish?

The CMI initiative will see all settings migrated to an xml flat file storage system with an obfuscated namespace so some of the security implications of this are less important but i still see value in simplifying things like htaccess, general security and ide configuration, etc. If theres a little more support ill spend the time to try it out in the next couple days.

So, reading through this issue, I understand what the problem is with having php files end in .module, .theme, etc., but the problem may not be as clear to other people.

Say I write a module that does something really cool that's key to my business...I dunno...maybe it makes all Microsoft products magically start working as intended or something.

Anyways, I hire some noob to put my site online for me. He uses lighty because it's faster and less bloated or something. Since core only has configuration files for Apache and IIS, the security precautions mentioned in #1 don't apply. So now, anybody on the internet could browse to http://myawesomesite.com/sites/all/modules/microsoft_magic/microsoft_mag... and see the code that powers my site.

This is bad.

So, I can see two possible fixes here:

• Stick .php on the end of any file that has PHP code in it (which makes sense in my mind); or
• Add configuration files for every web server that currently exists (or ever will exist). (Which is not a good plan: I'm being sarcastic.)

Since i wasnt particularly clear above , the other issues include the dx papercut level issues of configuring IDE's (im looking at you eclipse!) to parse .module/.install etc files as php so you get source formatting, error checking and code completion. And issues greping by file type:

grep -r --include=*.php "some_string" some_directory

+1 for this because of IDE configurations and because of "Stick .php on the end of any file that has PHP code in it (which makes sense in my mind)" from #17. Sometimes just making sense explains enough why this is the way to do a thing.

Note that since we've now adopted PSR-0 for OO code in core (and modules once we figure out how to make that work), it mandates a ClassName.php format anyway for OO stuff.

Initial moving for inc, module and theme extensions:

find . \( -name '*.inc' -o -name '*.module' -o -name '*.theme' \) | while read FILE; do git mv "$FILE" "$FILE.php"; done;

I did not find any file with a theme extension on core, I guess this should be related with the age of the issue :-)

Other files/extensions on core with php but not a php extension on the filename:

• core/themes/engines/phptemplate/phptemplate.engine
• *.test
• *.profile
• *.install

I mainly used changes on #22336: Move all core Drupal files under a /core folder to improve usability and upgrades as reference for this, and then start to solve some of the problems so submitting a format-patch output to see what test bot says.

Far for being completed, but it's an start.

Expanding on my meaning in #20... Since this will de-facto happen anyway as we convert stuff over to PSR-0, should we bother doing it for /includes at all at this point? Seems like that would break more patches than it needs to right now.

I'm not against this change in the general sense; I'm just asking if we shouldn't wait until it's a smaller problem space anyway.

Wow, this issue is close to celebrating its 9th birthday :)

This seems like a task to me rather than a feature request.

It's potential to break lots of patches is definitely relevant at any given time, but now that CVS is a thing of the past, I'm guessing that git rebase (potentially) makes that problem trivial?

Refactoring the theme system and #1033116: Rename template.php to THEMENAME.theme to eliminate ambiguity around "the template file" vs. "template files" brought me here.

Based on 21:

find . \( -name '*.inc' -o -name '*.module' -name '*.engine' -o -name '*.test' -o -name '*.profile' -o -name '*.install' -o -name '*.theme' \) | while read FILE; do git mv "$FILE" "$FILE.php"; done;

New patch. It will probably fail install. Note: when rolling patches for this, please include -C -M in your git diff arguments to keep the patch size down.

updated issue summary..lets make title a bit smaller:)

A proper issue summary

Hrm. If we change this, then let's change it for real.

1. Simple .php file extensions for the main extension files.
2. Double .php file extensions for additional/include files.

In other words:

Why?

Because anything else is duct-taping. Next to what @Crell stated about PSR-0 code in #20 already — the road we're actually running on is this:

1. #1033116: Rename template.php to THEMENAME.theme to eliminate ambiguity around "the template file" vs. "template files"
2. #1793074: Convert .info files to YAML
3. #340723: Make modules and installation profiles only require .info.yml files
4. #1292284: Require 'type' key in .info.yml files
5. #1398772: Replace .info.yml with composer.json for extensions

Can you see the light?

rename from core/vendor/twig/twig/test/Twig/Tests/Fixtures/tests/odd.test
rename to core/vendor/twig/twig/test/Twig/Tests/Fixtures/tests/odd.test.php

I think we should exclude core/vendor files from renaming process, right?

So I started working on fixing all the supporting code for a mass rename last night, and it's going to be a nightmare. There are over 1000 places in core that need to be changed to make the change work (basically, anywhere there is a module_load_include, form_load_include, require(_once), or include(_once)). I'm fine with sun's plan in #28, but this isn't really looking like a one-person job. Somebody want to work on this with me next weekend?

@cweagans count me in, i definitely want to see this in d8

I can see that.

What if we'd rename all files and change ModuleHandler::loadInclude() to always append a .php file extension though? Apparently that would also match my DX expectations — why should I have to manually specify .php of PHP include files? :)

(Likewise for module_load_install().)

Related: #1308152: Add stream wrappers to access extension files

(Abstracting away from the file name.)

Ohhhh, that issue that Crell linked looks pretty awesome. Should we try to get that one in first (before feature freeze) then revisit this?

Given http://drupal.org/node/1308152#comment-6284820, I actually don't think that issue helps this one much in this cycle.

What if we'd rename all files and change ModuleHandler::loadInclude() to always append a .php file extension

Sounds a nice solution, but i fear this would break the entire contrib :D

Edit: Actually nvm we already doing so..geez i need to sleep

curious to see what testbot says..in my local installation fails cause somehow module sorts are screwed in ModuleHandler::buildModuleDependencies..

About the patch..started doing the rename, and then i remember something i have seen in kohana..having .php defined as constant...so i thought why not..i am pretty sure EXT is not ideal..could be
- EXT(ENSION)
- PHP_EXT(ENSION)
- DRUPAL_PHP_EXT(ENSION)
or just hardcode it and not use it at all..i am fine with anything.

About #28 there are some problems:/ which might need a lot more changes..also statistics already has a statistics.php file somehow:/ i say we leave this for a followup and postponed to issues listed above

lets see this one..also removed the constant, i think it does not go inline with "do not hack core" since it makes you think you can actually rename files..this should go past the installer

yeap..scripts folder

uh, patch does not include GetFilenameUnitTest fixes..sorry testbot, cant cancel you

this should fix update.php, color module and batch exceptions

this should go green

Patch is green so I think it is good for RTBC. As long as we agree on foo.module → foo.module.php

Created #1927464: Remove .module and other drupalisms from extensions for #28
This patch introduce no API changes, it just renames files
Only change from a DX perspective is that you need to include .php when using (include|require)_once functions

Added .test and .engine files

Thanks for working on this! This looks great to me.

I'm really looking forward to kill all of the extra config for discovering Drupal-PHP files in my text editor.

+  $install_state['profiles'] += drupal_system_listing('/^' . DRUPAL_PHP_FUNCTION_PATTERN . '\.profile.php$/', 'profiles');

In all regular expressions, we need to escape the dot . through \. — otherwise, the dot would stand for any character.

I also think we want to adjust /.htaccess and /web.config accordingly, but it might be better to leave that to a follow-up issue, as it will likely need further discussion.

Likewise, we'll have to review all help texts and also .txt documentation files throughout core. That's going to be a daunting task, so we better defer that to a follow-up issue, too.

Also, #1033116: Rename template.php to THEMENAME.theme to eliminate ambiguity around "the template file" vs. "template files" happens to be RTBC, so you might want to wait for that to land, before re-rolling this.

Ah, yeah, this patch needs a reroll quite sometime now, but i wait for the issue above to land..
and thanks for the dot, totally missed it:)

Needs a reroll now that #1033116: Rename template.php to THEMENAME.theme to eliminate ambiguity around "the template file" vs. "template files" has landed...

Also...

# Protect files and directories from prying eyes.
<FilesMatch "\.(engine|inc|info|install|make|module|profile|test|po|sh|.*sql|theme|tpl(\.php)?|xtmpl)(~|\.sw[op]|\.bak|\.orig|\.save)?$|^(\..*|Entries.*|Repository|Root|Tag|Template)$|^#.*#$|\.php(~|\.sw[op]|\.bak|\.orig\.save)$">
  Order allow,deny
</FilesMatch>

Can be a lot simpler :)

Why keeping the .inc extension (e.g. bootstrap.inc.php), it seems redundant.

@pounard... Actually considering how much fun rolling this going to be I think we should merge with #1927464: Remove .module and other drupalisms from extensions and just do it all at once.

Fun++

merged 8.x
fixed the dot according to sun's comment
fixed htaccess and web.config (while i was there i noticed that yml module files are downloadable from your browser)
fixed .theme

note for next reroll..we should take theme out of htaccess and web.config

-removed theme from htaccess web.config
-renamed newly added test modules
-fixed update-iso-3166.sh

200kb

some new test modules sneaked in:)
yeah merging #1927464: Remove .module and other drupalisms from extensions in might be a good idea, but then should be postponed by #1292284: Require 'type' key in .info.yml files which touches all .info.yml files...so all of those 3 issues would be hard to roll..so merge all 3? we would have a giant patch, but would break other people patches by two less commits
not sure what to do

moved the new forum test module for views integration

Just before everyone dumps a HUGE amount of time into this, assigning to Dries to say yay/nay (this should probably be able to happen Tuesday).

If someone wants to touch up the issue summary a bit before then, to lay out the pros of doing this HUMUNGOUS patch :) that might be good.

From #12:

there is an advantage to not using .php since by default those files are not executable by the PHP interpreter

Would be great for the issue summary to address this. Are we more ok with poorly configured lighttpd, etc. executing .inc and .module files than we are with it displaying them, and if so, why? Executing PSR-0 class files is less risky, since those are required to be just class definitions, but I know there's contrib and custom modules out there with actual executable code (not just function definitions) in their .module and .inc files.

imo, having browser display API keys/password or any sensitive info when webserver is not executing .inc files, just displaying them, is far more worse than executing them and result to a fatal error, since most inc files are being dependent on module files and drupal core itself, which are not loaded, since those scripts where never meant to run alone...

Added followup link

Joomla used to (not sure if they still do) have some hack at the top of each .inc file saying basically:

"If the user's trying to load this directly, print 'you suck'"

We could do something similar I guess, but meh.

not only joomla..several frameworks too..like kohana:

defined('SYSPATH') or die('No direct script access.');

on top of each class...i would be ok with that but i agree with webchick, meh

Updated issue summary

remove .test files..this drupalism is gone now yay

summary updated:)

Issue summary V2

Minor fix

Headings

Ok, so here's the word of Dries (via e-mail, since he's currently at an event):

Sounds like a "feature request" that should wait until we're below thresholds.

I actually tend to agree. As much as I'd love to close a 4-digit issue, and as much as some good arguments have been laid out for why this makes sense to do, we do have many more pressing thing atm.. 21, 146, 38, and 138 of them, to be exact. :\

So it's not a "no" but it's definitely a "later" and if you want "later" to be "sooner," then please help us close majors/criticals. :)

And I guess...

As with other similar issues, I don't understand why renaming files is considered a feature request. It's not adding any new capabilities to Drupal. The fact that using the correct extension makes Drupal work better with text editors is, IMO, a bug fix and not a new feature, but even that is a shaky argument, so I'm very much in favor of this continuing to be a task. I won't change the category or status back, but I thought I'd voice my dissension in case anyone else agrees.

Mainly because it's a huge effing distraction that'll break everyone else's patches, and way less important than getting D8 ready to ship. So if we get D8 ready to ship, then we can look at "nice to haves" like this again.

Agreed. This would literally cause the high majority of applicable patches to fail. Later in the release cycle would be better, but I would still love to see this land. Do we have a tag for things that we need to have happen late in the cycle but before we need to start supporting HEAD to HEAD updates?

this is another good example why our patch-based system fails. there would be no issue if we were using PR's..but hey thats a totally different topic.
Created a sandbox here and now pushing all of my work, to make life easier to any future taker who remembers this issue when thresholds are down
http://drupal.org/sandbox/rootatwc/1969260

Related #1936886: Rename $module.info.yml into extension.yml

Updated issue summary.

#64: drupal-php_ext-7269-64.patch queued for re-testing.

+1 for the suggestion
Having to configure your IDE to figure out all these "non extension" items for Drupal is also a pain.

adding this tag

Updated issue summary.

There's been some interest inside Propeople around doing this. WIP patch attached. Feedback welcome.

To move files, run this:

`for f in $(find . -name "*.module" -o -name "*.profile" -o -name "*.theme" -o -name "*.install" -o -name "*.engine"); do git mv $f ${f}.php; done`

This just basically appends .php to all .module, .profile, .theme, .install, and .engine files. .inc files are not considered right now at all.

I intensively worked on the overall extension system recently. In particular in light of #340723: Make modules and installation profiles only require .info.yml files:

We would resolve some very hard problems if the .module/.profile/.engine suffixes would be removed from the main extension files.

Here is why:

Profiles are also registered as modules. All installed modules are registered on the Container as the %container.modules% parameter:

'container.modules' => array(
  'block' => 'core/modules/block/block.module',
  'field' => 'core/modules/field/field.module',
  'system' => 'core/modules/system/system.module',
  'user' => 'core/modules/user/user.module',
  'minimal' => 'core/profiles/minimal/minimal.profile',
),

In order to make #340723 happen, those paths will essentially be replaced with the .info.yml pathname of each extension. — But with the current .module vs. .profile mismatch, we'd have to implement some very weird/ugly code in various spots, in order to make "user" resolve into "user.module.php" and "minimal" resolve into "minimal.profile.php".

However, if those unnecessary extension type suffixes were dropped, then the main extension file resolution would be as simple as this:

$file = basename($info_filename, '.info.yml') . '.php';
var_dump($info_filename, $file);
// yields e.g.:
// core/modules/user/user.info.yml
// core/modules/user/user.php
// core/profiles/minimal/minimal.info.yml
// core/profiles/minimal/minimal.php

As long as we have .php on the end of PHP files, I don't particularly care how this issue is resolved, so how would you like to move forward?

;)

how about syncing this issue with #2247991: [May 27] Move all module code from …/lib/Drupal/… to …/src/… for PSR-4 and move it to a postponed task?
i would be happy to work on it and make sure it is ready immediately after the issue above is committed

Huge +1 to #89 And I would be happy to review it.

Re #89, @ParisLiakos, if we were to do that, this would need much wider consensus first. I'd suggest promoting this issue on g.d.o/core for feedback.

Myself, while I see that this looks good on paper, I cannot imagine justifying a change of this scope and disruption at this point in the cycle. (But then, I also still think that about the PSR-4 issue, so...)

PSR-4 will be a lot bigger distraction, now that most of our code lives in classes.
Patch in #64 was 200kb 1 year ago, and since then, a LOT .inc and .module files have died:)

I would be glad though to post on g.d.o/core for feedback, but i am afraid i do not have the permissions to do so

re: #67 / #12:

there is an advantage to not using .php since by default those files are not executable by the PHP interpreter

That problem space never really applied to Drupal, because PHP files of Drupal extensions are not supposed to actively execute PHP code on their own.

Historically, that is caused by the module hook architecture, in which directly executed PHP never existed, because extensions are able to cleanly participate even in early bootstrap via hooks like hook_boot() and hook_init().

Other application frameworks need to add that protection, because their PHP files are essentially used as front-controllers; i.e., a HTTP request does request the .php file of an extension itself, and only that extension-specific front-controller actively "loads" the application framework, which in turn requires to execute PHP code in the global scope.

In Drupal 8, most PHP code lives in PSR-0/4 class files, which do not need such a protection, because there is no executable code. Requesting the .php file of a class yields a blank page.

In addition, procedural PHP files for extensions only continue to exist to carry hook implementations. Because not every extension has a use-case for implementing any hooks, the procedural files have been made optional. There are still active attempts to (A) lazy-load procedural PHP files only when a hook gets invoked, (B) move all hook implementations into static callables in a Hook class, and (C) get rid of the hook system altogether by introducing an event dispatcher that natively resolves some long-standing architectural flaws of the hook system.

Long story short: Unless aforementioned options B or C will supersede this issue for D8, it is long overdue to fix the false-exposure of PHP files and requiring every frickin' editor and IDE to be manually configured to understand this Drupalism. There is no need for a direct code execution prevention à la WordPress, Joomla & Co, because PHP files of Drupal extensions do not execute any code in the global scope — any existence of such code is a preexisting bug/problem of a poorly/wrongly authored extension and should not hold up this change proposal.

@ParisLiakos: You are supposed to have access to the group anyway as a core component maintainer, so I've now granted it and you should be able to post there now. :) (Just keep in mind the group is intended for announcements, not discussions, so I'll disable comments on the post after you post it.) Thanks!

Created https://groups.drupal.org/node/420168 and also del'ing the Option 1 in issue summary, since we are now going with option 2

I think the disruption this causes this close to beta out weights and minor benefit we might have. -1

Please let's just fix this already. RE: it being close to beta, this is why stuff like this doesn't get fixed... like it should have years ago. It was pushed until later for a reason (#73), whether that reason is still applicable or not... unsure.

I'm +1 here, mostly on "don't waste my time with IDE configuration" grounds. My one concern with leaving out the .module. part of the file name is possible confusion with class files, as it then looks like it. Also "the module file" no longer has a visual indicator. There's no indication of why not to go with option 1.

Either way, I'm in favor of .php all the things.

That's rough but the group post asked for our opinion and that is mine. I'd prefer this to be marked priority for 8.1/9.x and stick with it rather then squeezing it in disrupting a bunch of other issues that, again in my opinion, are more important. We'll see how that shakes down with other opinions.

The renames here should be recognized by Git. Git can then work with older patches and apply them to the new filenames. If git fails there,I assume the cause is that folks keep uploading "dumb" patch files. Our patch guidelines should recommend `git format-patch` instead of `git diff` in order to overcome this very issue. We shoot ourselves in the foot avoiding "disruptive" issues when git has already solved this problem.

what crell mentioned about option 1 makes sense, i heard from others as well.
i went for option 2 mostly because of sun's comment and because option 1 seemed to verbose for me..but i am happy to bring back Option 1 for discussion..i am +1 for both options, dont care much, as long as there is .php everywhere in the end

When I want to look for the hook implementations, I am looking for .module.
If I need to look for the preprocess functions, I am looking for .theme.

Right now we have \Drupal\views\Views which is Views.php

If we rename views.module to views.php, that will confuse me and my IDE.

All of the points in support of adding .php are still valid if we leave .module/.theme etc.

So, +1 for either status quo or for Option 1.

Strong -1 for Option 2.

+1 option 1, removal of module|install is d9 material

I agree with tim.plunkett. I would like to see it still be obvious, when I am browsing through file directories, what is a "File containing exactly one class", which are named TheClassName.php, and what is a "main module file", and what is a "main theme file", and what is a "include file containing some constants and functions that should be grouped together". So I hate option 2, which jsut renames all of them *.php, and vastly prefer option 1, which names them *.module.php, *.theme.php, etc.

So by the way... We need to think about hook_hook_info(), which allows you to say which include file a hook goes into:
https://api.drupal.org/api/drupal/core!modules!system!system.api.php/fun...
We still have 3 real and 1 test implementation in D8 core.

And also the functions module_load_include() and module_load_install().

None of this is addressed in the latest patch, as far as I can see. I am going to add them to the issue summary in the "Proposed resolution" section, so that they don't get forgotten.

For what it's worth, I'd prefer option 1 at the same time as the PSR-0/PSR-4 transition (i.e.: before 8.x-beta1): if we've got to break all the patches, we might as well break them all at one time, instead of breaking them all twice.

I don't know that anyone really used hook_hook_info(), to be honest. And with so much code moved to classes and an opcode cache assumed in nearly all cases its original goal (reducing load time by loading less code) is not as compelling anymore. If it makes life easier to remove it I'd be fine with that. (I added it originally; I won't mind now if it goes away.)

There are 4 implementations of hook_hook_info() in Core. Notable is system_hook_info(), which (among other things) lets you put token hooks into a modulename.tokens.inc file. Several core modules make use of this.

I haven't looked through the other parts of system or the other implementations, but it *is* being used.

That said, I also think it's not a huge deal if the hook_token_info() and other things using hook_hook_info() move back to the module.php files.

Still, one way or another, we need to deal with it (either remove hook_hook_info() and move the contents of tokens.inc files and any other hook_hook_info() auto-includes back to the .module.php files, or make the auto-includes work) in this issue's patch.

hook_hook_info is not affected by this patch? everything works as should, just stick a .php in the end of the file and you are done.
also module_load_include() and friends, work as they used :) It is just the filename that changes. no other API changes

see also #50

Another +1 for Option 1.

I really hope that we get this into D8 and not postpone it so much that we have to wait for D9.

RE #110 - hook_hook_info(), and the code in ModuleHandler that deals with it, automatically (currently) loads a file called (module_short_name).(hook_group_name).inc. The code that does this, and the docs for hook_hook_info() that explain this, will need to be changed (small change) to instead load *.inc.php files. I'm not claiming this is a huge or difficult change, just that it will need to be done.

Same with module_load_include(), whose default should change to load a .inc.php file, or else all the calls to it will need to change so that they request the right file name.

I think it's probably a good idea although it sounds like a huge thing to do at this stage.

The real answer to the unintended direct execution or exposure problem is for the vast majority of the application to be outside of the web root. A typical Symfony application has a web folder containing just the front controller with static assets (usually symlinked) below it. That's where the web server points. It's absolutely impossible for anything outside of that folder to be requested by the web server. No protection with .htaccess or magic variable that must exist is required.

+1 on either option! :-)

Huge fan of Option 1 :) +1

A code review note:

+++ b/core/modules/system/lib/Drupal/system/Tests/Common/SystemListingTest.php
@@ -48,17 +48,17 @@ function testDirectoryPrecedence() {
+    $files = drupal_system_listing('/\.module.php$/', 'modules');

A few of these little regex gotcha's in both patches in #78 & #85 @cweagans and @ParisLiakos.
Though this will work/pass the dot needs an escape.
+ $files = drupal_system_listing('/\.module\.php$/', 'modules');

Cheers, thanks for championing this and I hope this gets though!

@jhodgdon: Thanks you are right, i completely forgot to change the docs :) i did it though in this patch
@joelpittet: thanks, good catch, but i believe we got rid of the need to change those in this new patch

seems Option 1 is a clear winner here, so i just merged 8.x and rerolled the last patch from one year back.. it is 70Kb smaller:) we did pretty good job all those months, getting rid of .inc files. it is going to fail, but i just want to see how bad.

I also noticed and opened #2253109: Followup: Bring .htaccess and web.config up to date while i was removing entries from .htaccess

Drush doesn't seem to like something that changed. Does there need to be a corresponding change to Drush too?

Also I notice two extra files:

 core/modules/statistics/tests/modules/statistics_test_views/statistics_test_views.module.php
core/modules/user/tests/modules/user_custom_phpass_params_test/user_custom_phpass_params_test.module.php

And looks like core/modules/update/tests/aaa_update_test.tar.gz changed but not sure if was meant to.

All the rest looks good. Reviewed patch modifications with:
git diff origin/8.x --diff-filter=M --color-words

ah thanks good catch! fixed them
about aaa_update_test.tar.gz i had to extract it and either rename its module file to module.php or delete it (since it is empty)
I picked the second:)

i ll shift my focus now to fix the issues required for the drush dependency to be removed

This is an absolute no go from a security standpoint as long as the PHP deny lines in .htaccess are commented out. If you want every piece in Drupal to carry .php then make those lines live and remove the comment signs.

I dont know if this is in scope for this issue or not, but all the scripts in the scripts folder, with the exception of cron-curl.sh and cron-lynx.sh are *also* php files. I dont know the history as to why they have a bash script extention, or whether they should also end in .php, but it definitely confuses my editor to open a .sh file and have php inside. Perhaps this is related to chx's point above?

Yes. Absolutely. We can rename those when the ban covers those. They are the best example of why the ban is necessary: something now happens in a global scope that previously did not happen. You don't know what. You either be very very sure every possible file is secure (glhf) or just ban them all from Apache.

Edit:

  # RewriteCond %{REQUEST_URI} !^/core/[^/]*\.php$
  # RewriteRule "^.+/.*\.php$" - [F]

These two will keep you safe and secure once the # signs are gone.

I don't know that this is that big a deal. If we do 1) we'd probably just do (\.php)? after the relevant files in the FilesMatch.

The rules are already in .htaccess ; they are merely commented out. Changing them is perhaps another issue.

Well we'd need to allow rebuild.php to pass through that htaccess check, otherwise it wouldn't work anymore. :-) I've no major objection to uncommenting those lines though other than that caveat.

ignore me

Re #125 rebuild.php is core/rebuld.php which is covered. Again: the rule is solid it just need to be made live. (edit: #126 is not a ragequit because i got involved merely it was a post to the wrong issue.)

+1 and minor review comment:

+++ b/core/includes/common.inc.php
@@ -2802,20 +2802,20 @@ function drupal_valid_token($token, $value = '') {
+  require_once __DIR__ . '/../../' . Settings::get('path_inc', 'core/includes/path.inc') . '.php';
...
+  require_once __DIR__ . '/../../' . Settings::get('menu_inc', 'core/includes/menu.inc') . '.php';
etc

IMHO, if the file names are from settings, then should get the full name (including .php).

+1 to the whole idea. I definitely favour Option 1, as the cost of a few extra characters makes a huge improvement in documentation/usability.

It is a right pain to configure Eclipse to tell it to treat .module and .inc as .php. The first time I used Eclipse with Drupal I was stuck with dull grey text for weeks (months?) because I didn't know how to set Eclipse up to understand. There are no doubt beginners out there who still have no PHP colours in Eclipse because they haven't figured it out. Over the years I must have had to do this dozens of times (installing different versions of Eclipse on different PCs etc.), so this is a big +1 with the whole idea.

I generally approve of the idea, and strongly prefer option #1.

+1, with a preference for option #1.

+1 for option 2. The shorter the better as you should be able to understand the use of that file through the folder context (PSR-4).
I'm also cool with option 1 as long as we get rid of ".confuseme" file extensions. It has PHP code inside, so it should have the .php extension.

johannez: This is for files with procedural code; PSR-4 is entirely irrelevant here.

It looks like we have a clear consensus here for moving forward with option 1 (switch to .php for everything but keep .module.php etc. for clarity). The next step would appear to be someone rerolling #116, taking chx's points from #120/#122 into account.

yes, i updated the issue summary, option 1 is the clear winner here.
i merged HEAD, but still..Drush needs to be updated for this change https://github.com/drush-ops/drush/issues/631, or #2206501: Remove dependency on Drush from test reviews to happen..so i am postponing this issue, till one of those happens

> taking chx's points from #120/#122 into account.

just found out that chx's points have a separate issue

Discussed with xjm and alexpott. If this happens in D8, it needs to happen before RC. Otherwise, it's a D9 task.

@cweagans - does this mean with beta 1 out, this change is off the table until D9?

I think it means it can still happen during beta but can't happen once the first RC comes out. Beta target, RC deadline.

Now that #2206501: Remove dependency on Drush from test reviews is complete this issue can be unpostponed as per #135 ParisLaikos' comment.

its not complete, its still rtbc

#2206501: Remove dependency on Drush from test reviews is now fixed.

i am curious

We have some fails, nice! lets fix some exceptions/fails

reroll

meh

OMG so close @ParisLiakos++ woo! meh++

This patch is in desperate need of a Beta evaluation, since it is a Normal Task and I think it's likely to be classified as Disruptive. Also at this point it is not a beta target -- that has come and gone.
https://www.drupal.org/contribute/core/beta-changes

Per https://www.drupal.org/node/7269#comment-8833387, it's still a possibility for D8. Just has to land before RC. It's clearly disruptive, but as long as we schedule it, it should be fine, right?

Either way, it needs a beta evaluation section of the issue summary. It's definitely disruptive. We need to understand what we gain from doing it.

I'd like to point out that the patch is only disruptive because we choose to use a patch workflow. Imagine the reduction in rerolls if we use merge instead. See http://www.mosheweitzman.me/post/96720635168/improve-drupal-org-code-wor...

@moshe: that would only be true if we'd ignore disruption for Drupal 8 contrib / custom code.

Maybe it's possible to script this, so contrib can easily migrate, or preserve BC for old files and clean-up at RC

I cant see a way to preserve BC without adding a bunch of file_exists calls.
I agree it is disruptive in terms that it ll break most contrib modules, but its pretty easy to unbreak them, just by renaming a few files (most modules is gonna be 1-3 max), even non-coders can do that. My point is, that no code will need to be changed, with this change :)

OK. For any patch that you actually want committed at this time, you need a beta evaluation added to the issue summary. And because it's an API change, you need to make a draft change notice. Neither of these has been done, and they do need to be done before the issue can be marked RTBC.

Regarding disruption, this patch will disrupt every contrib project that exists with an 8.x branch, because you will force all of them to rename one or more files, and their modules will not work at all until they do so. Yes, it's easy, but that does not mean it's not also disruptive.

I'm not saying it's too disruptive to be committed right now; that is not my decision to make. I'm just saying that this disruption needs to be described in the change notice and the beta evaluation block.

I have one other gripe: the issue summary. Several of the "Reasons to do" arguments do not really apply:
- "Drupal is part of the PHP world" says to look at core/vendor and notice all the files there end in .php. I would just like to point out that all the PHP files in there are class files, and all of our class files already end in .php -- in both cases, they have to because of PSR-0/4. So this argument is not really valid as it stands.
- "It's going to happen sooner or later" -- Uh. The argument in there doesn't equate to "it's going to happen". The header is misleading. It's not necessarily going to happen. All of our class files already end in .php, but that does not mean that the non-class files will end in .php for sure.

So I've taken the liberty of editing the issue summary slightly. I combined the "Drupal is part of the PHP world" with the first point, and renamed the other one.

tag weirdness, I think there is a bug in the auto-complete field

This is RTBC for me. Added change notice https://www.drupal.org/node/2425065
As per https://groups.drupal.org/node/456108 "Drupal 8 beta 7 on Wednesday, February 25, 2015". Let's commit it just before beta for minimum disruption.

thanks Jibran! i also added the beta evaluation. Now its up to commiters to decide

#2414255: Subtheme template inheritance working in reverse order added a new .theme file, so just moving that one too

Can remain RTBC. Testbot will inform otherwise.

OK... So, take a look at the flowchart on:
https://www.drupal.org/core/beta-changes
Given the current Beta Evaluation block, and the current issue priority/category, this change will most likely be postponed to at least Drupal 8.0.x, and I think if it gets that far, it will be put off until later yet (9.x) due to it breaking contrib modules.

It is currently a Normal Task, and there is nothing in the Beta Evaluation block saying that it is either Unfrozen, Prioritized, or Reduces Fragility. The flowchart says that if it is not Critical or Major or Unfrozen or Prioritized or Reduces Fragility, it has to be postponed.

Therefore, if you want it to actually get into 8.0, you'll need to come up with a reason that it falls into one of these areas. So setting back to NR until that is added to the Beta Evaluation.

Reduces fragility: When installing Drupal on a server that's not Apache or IIS (since that's what we ship config for), having a .php extension more or less guarantees that code won't be exposed to the end user. It'll be handled like a normal PHP file, and that's the *right* behavior because that's what it is. If we want to go really crazy, we can call it a bug report ("PHP file contents displayed to users") and have the proposed solution be "Rename all this crap to the proper extension so we're not in our own little world anymore".

This is clearly a disruptive patch. Pushing it off again isn't going to change that. There's good arguments for doing this, and the only argument against it is that it's going to break a bunch of patches. There's never going to be a time when it's not disruptive. Ever. We should just commit it and finally be done with it.

And for that matter, you could even call it a security issue (possible information disclosure if you have API keys hardcoded somewhere for some strange reason).

You need to put whatever arguments you want to be considered into the Beta Eval block. ;)

Oh for the love of all things... enough about the beta evaluation....

I really fail to see why it needs one in the first place when an "RC deadline" tag was already added in #139.

Regardless, I updated it to appease.

----

This issue is clearly no longer about what or how we do it, but when?

@catch clearly outlined 3 huge wins all the way back in #8 from 7 years ago which is also when the issue's version bumped to 7.x from nothing...

Considering that this actually never made it into 7.x are we really going to keep pushing this until..... when exactly??

There's never going to be a "good time" to do this, but it makes a lot of sense considering all that has been done in 8.x and many using IDEs like PhpStorm.

From the comments #72 and onward, I think the general idea was to revisit this right before an RC release (which clearly means right around now).

The reasons, from my perspective, that it didn't happen back then was because there was still way too much going on.

Files were changing left and right and it would have severely threw a wrench into things.

Now that things have more or less "settled down" so to speak, this seems like now is a good time as any.

Am I the only one that experiences the notion that core has basically been rewritten in 8.x, but changing PHP files with extensions from a Druplism to a .php standard is way too much (when it really isn't)? Hilarious lol

Can we at least not let that be one of the things we're laughed about???

Lets just rip the band-aid off already and do it...

#176 Exactly, well put, thanks for the beta eval @cweagans and @markcarver.

Rip off the band-aid!

This needs to happen before any kind of beta-to-beta upgrade path is supported.

About the disruption: i am upgrading 3 small modules from drupal 7 to drupal 8, and i do not remember any beta that my modules do not need some change at last small to work with next beta, just as example, there was some disruption when config objects was made imutable from beta 5 to beta 6.

This will require less than 5 minutes to rename some files for almost all contrib modules, so +1 to make it happen now.

+1

#ripoffthebandaid

thanks everyone for your comments! i think this time we are close making this happen.

#2338559: Never serialize password fields by default introduced one more .module file, just renaming it

We will also need a follow-up to update the module/theme/profile developer guides with this new convention for the .module, .theme, .profile files for D8.

@jhodgdon Stubbed out that follow-up and postponed. #2426145: Update the module/theme/profile developer guides with this new uniform .php extension convention.

#ripoffthebandaid ++

I've hit a breaking change with every alpha and beta release with Honeypot, so let's keep that record going with the next beta ;-)

This is one of the few breaking changes that I'm 100% in support of, though. Hearty +1, and will gladly spend a few minutes tagging another new release pre-RC.

Easy rebase.

Adding .engine to the table in the issue summary.

Something like:

tree core | grep -E "\.(engine|inc|install|module|profile|theme)$"

can be used to check for any stragglers.

Fix after #2417733: Drupal 8 breaks Twig's round filter. was committed.

I don't want to be the person to hold this up, and I think it's totally fine that \Drupal::moduleHandler()->loadInclude()/module_load_include() doesn't need the .php extension, but…

1. +++ b/core/includes/batch.inc.php
   @@ -231,8 +231,8 @@ function _batch_process() {
   -    if ($set_changed && isset($current_set['file']) && is_file($current_set['file'])) {
   -      include_once \Drupal::root() . '/' . $current_set['file'];
   +    if ($set_changed && isset($current_set['file']) && is_file($current_set['file'] . '.php')) {
   +      include_once \Drupal::root() . '/' . $current_set['file'] . '.php';
   
   @@ -404,8 +404,8 @@ function _batch_finished() {
   -      if (isset($batch_set['file']) && is_file($batch_set['file'])) {
   -        include_once \Drupal::root() . '/' . $batch_set['file'];
   +      if (isset($batch_set['file']) && is_file($batch_set['file'] . '.php')) {
   +        include_once \Drupal::root() . '/' . $batch_set['file'] . '.php';
   

   I think this means that the 'file' key in batch arrays now needs to be documented as "without the .php extension". Personally I'd rather volunteer to update all batch arrays than have this, I can imagine this being a nightmare to debug if you make the fair assumption that the 'file' array key contains the full filename.

2. +++ b/core/lib/Drupal/Core/Theme/Registry.php
   @@ -431,7 +431,7 @@ protected function processExtension(&$cache, $name, $type, $theme, $path) {
   -          include_once $this->root . '/' . $include_file;
   +          include_once $this->root . '/' . $include_file . '.php';
   

   I feel more strongly about this one as a theme system maintainer: If I'm not mistaken this means that the 'file' key in hook_theme() would need to be documented as "the file name without the .php extension". Again, I'd rather volunteer to update all hook_theme() definitions than have hook_theme() be more confusing than it already is.