8000 use .php.cache for all php files generated into the cache dir by lsmith77 · Pull Request #511 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

use .php.cache for all php files generated into the cache dir #511

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

lsmith77
Copy link
Contributor
@lsmith77 lsmith77 commented Apr 8, 2011

similar to the bootstrap.php.cache this prevents IDE's from indexing these files

@stloyd
Copy link
Contributor
stloyd commented Apr 8, 2011

👍

@henrikbjorn
Copy link
Contributor

Isnt it implicit that the files in the cache dir are caches? And i your ide picks them up and you cannot set some setting to prevent it i would suggest you go find another editor.

@marijn
Copy link
marijn commented Apr 8, 2011

On a related note, is there a reason why bootstrap.php.cache is generated into the app directory instead of app/cache?

@marijn
Copy link
marijn commented Apr 8, 2011

P.S. I tend to agree with @henrikbjorn

@lsmith77
Copy link
Contributor Author
lsmith77 commented Apr 8, 2011

Sure I can configure it to ignore, so this is for convenience. Where is the harm though in this change? I see none and we already did it for the bootstrap.

As for the bootstrap, the reason is that it is intended to have a very different "lifetime". Aka its only updated if you update your vendors. Speaking of which, we should probably clear the cache dir after running vendor.sh. But thats another topic.

@henrikbjorn
Copy link
Contributor

@lsmith77 the reason for not changing is the same as the one with template naming where the last part refers to the filetype. If you add .cache you will not refer to a .php file anymore which is wrong. Also if people have very insecure installations (some does) the .cache files will not be intepreted as php but shown in clear text.

Furthermore this is a pull request that will only make it easier for you. And this is only a problem because you choose to use a editor that cant handle ignoreing somefiles for it autocomplete.

@lsmith77
Copy link
Contributor Author
lsmith77 commented Apr 8, 2011

There is little reason to consider these files as "php" files. Very rarely if ever users will look into these files at all, they should never be manually be modified. As for solving a problem "only for me", if by "you", you mean people like me, the first commenter and everybody else infavor of postfixing the bootstrap file with ".cache", then you are right.

@marijn
Copy link
marijn commented Apr 8, 2011

@lsmith77 I disagree, if you get an error message early in the bootstrap process an exception can be thrown referencing the file in the stack trace.

@lsmith77
Copy link
Contributor Author
lsmith77 commented Apr 8, 2011

Yeah, thats the rare case I was referring to for reading the files. Anyway the point of this pull is to be practical. IMHO, and I am not alone here, its more practical to use a php.cache extension. While the file can be hidden manually by configuring the IDE, so can php.cache be added for syntax highlighting if its needed. That being said I think the bulk of the users will not need the syntax highlighting, but appreciate not polluting their class searches in their IDE.

However if this isnt merged, then we should also drop the php.cache extension for the bootstrap file.

@henrikbjorn
Copy link
Contributor

@lsmith77 we can both agree that the files contains php code. And therefor they should end with a .php file extension. Why would it be beneficial to change the extension (other than you editor is wierd) ?

@lsmith77
Copy link
Contributor Author
lsmith77 commented Apr 8, 2011

lets just agree that we disagree. but either way its not the end of the world. either we will have a vote or fabien decides.

@pborreli
Copy link
Contributor
pborreli commented Apr 8, 2011

Hmm already thought about that and still dunno if it's good or not, here's my thoughts :

👍

  • IDE indexing
  • CI tools (won't parse those files and won't generate crazy stats)

👎

  • IDE editing (having those files interpreted as php is not good for IDE autocompletion for example, but it can be great for debugging to detect fatal errors or warnings or even browse from class to class)
  • Is that really a problem as you can define easily your cache folder anywhere you want so putting it outside the project will resolve this issue

After all, i think i'm still 👍 but i'm not against listening more arguments :)

@Seldaek
Copy link
Member
Seldaek commented Apr 9, 2011

I'm definitely 👍 - the only use case that it disturbs is people trying to debug issues in the framework itself, which in time should hopefully only affect core devs, and if you really need your syntax highlighting you can disable the bootstrap file in your app_dev.php and just require the autoloader afaik. Of course for looking at compiled DIC and stuff it's not as nice, but again this is a use case that should be less and less common with time.

On the other hand, people with no Sf2 experience coming to it with their IDEs and opening cache files by mistake sounds pretty bad to me. I saw how it looked on @lsmith77 machine it's pretty annoying every time you want to jump to a class that's in cache it offers you two choices and you have to select one or the other. Beyond the fact that it's confusing, it's also slower, and it occurs much more often in day to day business.

@henrikbjorn
Copy link
Contributor

There is no argument here listing any benefit other than it would be nice for people using IDE's when doing development.

I am a strong believer that the file extension should match the file type, otherwise how would you know what content the file has ?

@schniper
Copy link
Contributor
schniper commented Apr 9, 2011

IDE support is a strong enough argument. I wouldn't want to program an enterprise scale application in notepad or without accurate code assist ;)
Naming the files .php.cache gives a strong hint about the file type, don't you think?
Most IDEs support excluding a directory from indexing; yet setting this would be a required task for each new project. Beginners will bash their heads around until figuring this out, etc.
If there's a way for making things simple out of the box, I'd say go for it.
👍

@henrikbjorn
Copy link
Contributor
8000 henrikbjorn commented Apr 9, 2011

@schniper i guess that a lot of people on unix / os x / windows will user notepad++ textmate coda vim gvim macvim emac aquamacs sublimetext... The list goes on and on, the list of ide arent very long there is Zend Studio (incl. Eclipse), PHPStorm, Netbeans and i cant think of any other.

And saying that you need to have code assist etc to create a "Enterprise scale application" is plain stupid but we can ask the OpenSky guys what they use ?

@schniper
Copy link
Contributor
schniper commented Apr 9, 2011

Once you reach a certain code size, you will appreciate the "little things" an IDE provides: templating, documentation, autocomplete, integrated debugging, in a word: standardization. var_dump and die doesn't cut it anymore.
Anyway: ignoring IDEs because not everybody uses them doesn't seem a very bright argument either. Programming in an IDE doesn't make you a lesser programmer.
And I do stick to my point: you will not see mission critical applications built in notepad++ :)

@henrikbjorn
Copy link
Contributor

@schniper i dont think that it is true that "when you reach a certain size" that is bullsh** and not even related to the issue and the argument is still the same "we are lazy and dont know how to setup our editor"

@schniper
Copy link
Contributor
schniper commented Apr 9, 2011

That is exactly the point: why not be lazy with this and use the time needed to setup/figure how to setup your editor/IDE for the actual programming?
The cache folder should be a semi black box anyway. Apart from some debugging, you'll seldom end up in there.

@henrikbjorn
Copy link
Contributor

Because the file ending is wrong and insecure for people who does not know how to setup webservers proberly. A php file will be intepreted and show a blank page, a cache file will show the source code. So it is to protect users who dosent know what they are doing and keeping the right filename for files. .cache does now tell you what is inside it a php extension tells you it contains php code. You dont download music files as .jpg because someone thought they liked that file extension better.

We could also go the whole way and just name all the files in the frame work with .inc and see how that goes.

@schniper
Copy link
Contributor
schniper commented Apr 9, 2011

See? There's a config misunderstanding :D Only the web folder should be web-visible.
The other files are not directly callable and thus pretty safe to use, no matter the extension.
And as this convention is already used for the bootstrap, for the same purpose I suspect, I really don't see the impediment.

@henrikbjorn
Copy link
Contributor

I also believe it is wrong for the bootstrap files and if you cant figure out how to setup your editor then you should learn the tool you are using. And if you want to be lazy and do programming why are you then using an editor where you have to setup projects, use your mouse to click to open files etc? Also sounds like a sucky editor if you cant set it to ignore some file globally based on a project root.

Yes it is a config misunderstanding but it will inevitable happen sometime when a new user trying out the framework fails to setup his/hers test enviroment up wrong and in my book it is more important to have people more safe even when they fail to setup webservers wrong than being lazy.

And your best argument is still "I am lazy and dont want to change editor settings."

@schniper
Copy link
Contributor
schniper commented Apr 9, 2011

Right. And I think it will stick :) Anyway, we've wasted enough scrollbar on this. Let's wait for other opinions.

@duanegran
Copy link

Maybe the answer is to have this as a configuration option? Then we could argue about which default to use. :)

I'm in favor of naming them .cache. It seems more useful to most users.

@schmittjoh
Copy link
Contributor

I'm also +1 on this, especially for the compiled class files.

@henrikbjorn, surely you can tell your editor to display these files with the proper markup even if they do not have the .php extension.

@henrikbjorn
Copy link
Contributor

@schmittjoh surely you can configure your editor to ignore these files so they dont show up in your code assit.

@schniper
Copy link
Contributor

C'moon :D The question is: which one of these 2 cases mostly impedes starting up your development: inconsistent code assist throughout your project or lack of syntax highlighting for some files you don't need to view anyway?

@hidenorigoto
Copy link
Contributor

FYI: related PR
#525

PR525 is focusing on more debugging issue.

@arghav
Copy link
Contributor
arghav commented Apr 20, 2011

I'm +1 on this.

@naderman
Copy link
Member

I think that hardcodng this decision in the proposed way is even worse than the decision itself. If I want to make a Symfony2 app available (with php config) that has no security issues by default when deployed wherever, then I now have to replace various components and copy&paste large methods into my subclasses to be able to change this back. So if this has to happen, at least make it more easy to configure back to the old way.

@lsmith77
Copy link
Contributor Author

The security argument simply holds no water. If your cache dir is exposed, so is your config dir. Its a non argument. The only somewhat valid argument is lack of syntax highlighting if you do need to read those files, but as stated before this requires a one time setting in your editor/ide vs. a per project setting.

Making this configurable doesnt seem feasible or necessary.

@jmikola
Copy link
Contributor
jmikola commented Apr 20, 2011

And saying that you need to have code assist etc to create a "Enterprise scale application" is plain stupid but we can ask the OpenSky guys what they use ?

@henrikbjorn, we have people using stuff ranging from from vi to eclipse. I've personally had no luck excluding these files from my build path. Although my project settings have:

  • Included: (All)
  • Excluded: opensky/cache/*

Eclipse continues to call up 4-5 copies of core classes with its auto-complete. One of those is the real file and the rest are duplicates from the multiple cache files. I'm unsure if this is the result of my own error with the configuration or some fault of eclipse.

@naderman
Copy link
Member

@lsmith77 You did see how I mentioned that any project which needs this as a security measure would also need PHP configuration? To me personally it's just not acceptable to deliver a project that requires more than dropping it into the webroot. The security problem is very real in this situation. I understand your points, and thus all I'm asking for is a way to configure this for such projects.

@lsmith77
Copy link
Contributor Author

If you want to be able to drop a Symfony2 app into a webroot you have to do add an .htaccess file (and enable .htaccess) into all subdirs preventing access, which again means one .htaccess for your app dir. Meaning once again this issue is not relevant to the topic discussed here because the cache dir is inside the app dir.

@naderman
Copy link
Member

I can neither assume that Apache is being used nor can I assume that .htaccess files even work. So yes, it is relevant. There are certainly a number of things to look out for, but this is one of them.

@lsmith77
Copy link
Contributor Author

Its not relevant because you need to secure the entire app dir. "Securing" just a few files from the cache dir by insisting to keep the .php extension gets you no where. So its irrelevant because you will need a more complete solution for this and this more complete solution isnt a patch work of small solutions. Its having to provide some mechanism to convince the server in question to only serve stuff that isnt in a number of directories (app, src, bin, vendor ..). It has nothing to do with the cache dir, let alone a certain type of file in the cache dir.

@naderman
Copy link
Member

There is simply no way to reliably accomplish that with all webservers, so the only viable solution is to make sure that critical information remains hidden in PHP files - be that in the app dir or the cache..

@lsmith77
Copy link
Contributor Author

This is security by luck. Obviously you will then have to use php for configuration too. Furthermore you have to hope that no Bundle gets the idea of caching anything without a .php extension etc, which means you will need to educate your users what Bundles they are allowed to use etc. I think if this is your solution, then you have a problem aka this is not a solution.

@naderman
Copy link
Member

There simply is no other solution. Of course you ship an .htaccess file and try to educate users about server configuration. But neither of those is guaranteed to work so going this extra step is vital. If you don't it'll be your fault for providing them with insecure software. It's not like anyone cares if you told them to configure their webserver ...

@schmittjoh
Copy link
Contributor

Maybe as a compromise we can have .php.cache in dev environment, and
.cache.php in prod environment?

On Thu, Apr 21, 2011 at 3:31 PM, naderman <
reply@reply.github.com>wrote:

There simply is no other solution. Of course you ship an .htaccess file and
try to educate users about server configuration. But neither of those is
guaranteed to work so going this extra step is vital. If you don't it'll be
your fault for providing them with insecure software. It's not like anyone
cares if you told them to configure their webserver ...

Reply to this email directly or view it on GitHub:
#511 (comment)

@lsmith77
Copy link
Contributor Author

Well at that point its cleaner to make it configurable. However what we are talking about here is not real security. Its patch work security where you try to make it harder. But its not security in the sense that the final solution is actually secure since there is plenty of stuff that doesnt end in .php still.

That being said my motivation to work on fixing such a use case is pretty low and I think the issue has been demonstrated. So if you think its an acceptable solution to "security" I invite you to write a patch to make the extension configurable.

@lsmith77
Copy link
Contributor Author

OK, I want this fixed, so I have started working on making this configurable. I am mostly done, however before I create a pull I have a question:
The main issue I want fixed is the "classes" cache files, since these overlap with classes defined in the framework itself. Now this is less of a problem for the container, router, assetic, twig caches.

In other words the question:

  1. use .php.cache only for files that aggregate code originally defined else where
  2. use .php.cache for any file that currently uses a .php extension?

The other question is should this parameter be defined inside the Kernel? If we go with 1) the parameter would only be needed inside Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\AddClassesToCachePass

@lsmith77
Copy link
Contributor Author

I have implemented 1) in #633

@lsmith77 lsmith77 closed this Apr 23, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

0