Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

ProxyFactory creates proxy's parent structure if it doesn't exist #162

Merged
merged 5 commits into from

4 participants

@ericclemmons

Wheeee, nested proxies can be generated without hassle!

lib/Doctrine/ORM/Proxy/ProxyFactory.php
@@ -152,6 +152,11 @@ class ProxyFactory
$file = str_replace($placeholders, $replacements, $file);
+ $parentDirectory = dirname($fileName);
+ if (! file_exists($parentDirectory)) {
@Koc
Koc added a note

!is_dir would be better

Fixed in fde9d12

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@beberlei
Owner

Check is writable pls and otherwisethrow eyception

@ericclemmons
@beberlei
Owner
@ericclemmons

@beberlei

I made another update. The problem with is_writable is that if the proxy directory is deeply nested, AFAIK you would have to traverse up the structure to the deepest existing directory, check that, then continue on.

Isn't mkdir($dir, ..., true) == is_writable($dir) in this situation? As in, if mkdir fails, then the directory cannot be created.

BTW, tested using Symfony2 and it works as expected

@ericclemmons

Updated the logic a bit more:

  1. If directory doesn't exist, an attempt to create the path is made
  2. If directory exists (or was just made), it's checked for write permissions; an exception is thrown if not writable

I'll rebase once we agree on implementation.

@beberlei
Owner

I see what you mean. This is ok then. Did you run the tests with this? You dont need to rebase, the PR is mergable for me from the interface.

lib/Doctrine/ORM/Proxy/ProxyFactory.php
@@ -152,6 +152,20 @@ class ProxyFactory
$file = str_replace($placeholders, $replacements, $file);
+ $parentDirectory = dirname($fileName);
+
+ if ( ! is_dir($parentDirectory)) {
+ try {

Did extensive testing and found that if a grandparent folder was owned by root, for example, then mkdir would throw an ErrorException, which I catch here. (Not sure if you prefer Exception or not)

@stof
stof added a note

mkdir does not throw an ErrorException but a PHP Warning or Error (don't remember which one). You get an ErrorException because of the Symfony2 ErrorHandler which is registered in debug mode

Cleaned up commit history thanks to @stof. Now we should be good :)

@stof
stof added a note

Well, you should still handle the error better than letting the PHP Warning appear. See the Symfony code creating the cache dir in the Kernel for instance

I'm normally against error suppression via @, but if It's Symfony-sanctioned, I'll update this logic to match...

See 5b64dbe: uses the same logic, but with slight CS change to match the rest of ProxyFactory. (Trying to maintain the same accent the rest of Doctrine uses)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ericclemmons

Updated the code just a little bit after extensive testing.

All tests pass: OK, but incomplete or skipped tests!

If the proxy directory is cache/dev/doctrine/orm/Proxies, I tested the following:

  • cache/dev owned by user. mkdir works as expected.
  • cache/dev/doctrine created & owned by root. is_dir works & ErrorException is caught, throwing a ProxyException
  • cache/dev/doctrine/orm/Proxies created & owned by user. file_put_contents works as expected
  • cache/dev/doctrine/orm/Proxies created & owned by root. is_writable works and a ProxyException is thrown.

Anything from this point on will mostly be cosmetic.

@ericclemmons ericclemmons Added error suppression to mkdir in ProxyFactory
See: Symfony\Component\HttpKernel\Kernel#buildContainre
5b64dbe
@ericclemmons

This is ready for final approval.

@beberlei beberlei merged commit 6b1ef08 into doctrine:master
@elHornair elHornair referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
4 lib/Doctrine/ORM/Proxy/ProxyException.php
@@ -36,6 +36,10 @@ public static function proxyDirectoryRequired() {
return new self("You must configure a proxy directory. See docs for details");
}
+ public static function proxyDirectoryNotWritable() {
+ return new self("Your proxy directory must be writable.");
+ }
+
public static function proxyNamespaceRequired() {
return new self("You must configure a proxy namespace. See docs for details");
}
View
10 lib/Doctrine/ORM/Proxy/ProxyFactory.php
@@ -152,6 +152,16 @@ private function _generateProxyClass($class, $proxyClassName, $fileName, $file)
$file = str_replace($placeholders, $replacements, $file);
+ $parentDirectory = dirname($fileName);
+
+ if ( ! is_dir($parentDirectory)) {
+ if (false === @mkdir($parentDirectory, 0775, true)) {
+ throw ProxyException::proxyDirectoryNotWritable();
+ }
+ } else if ( ! is_writable($parentDirectory)) {
+ throw ProxyException::proxyDirectoryNotWritable();
+ }
+
file_put_contents($fileName, $file, LOCK_EX);
}
Something went wrong with that request. Please try again.