Doctrine 1

Doctrine_Migration_Diff reuses the same temporary folder on consecutive runs, resulting in collisions between projects

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Critical Critical
  • Resolution: Fixed
  • Affects Version/s: 1.2.0
  • Fix Version/s: 1.2.1
  • Component/s: Migrations
  • Labels:
    None
  • Environment:
    Symfony 1.3 with MacPorts PHP 5.3.1

Description

svn co http://svn.symfony-project.com/plugins/pkContextCMSPlugin/sandbox/branches/1.3 cmstest13
cd cmstest13
[edited config/doctrine/schema.yml and added a trivial table so there would be something at the app level]
[successful doctrine:build --all followed]

./symfony doctrine:generate-migrations-diff
>> doctrine generating migration diff
>> file+ /private/var/folders/3H/3Hu3TTyjFt...TI/Tmp/doctrine_schema_15549.yml

Fatal error: Class 'EventUser' not found in /private/var/folders/3H/3Hu3TTyjFtuvtN3D5tDUxU+++TI/Tmp/fromprfx_doctrine_tmp_dirs/base/BaseEventGuest.class.php on line 7

I recognize my personal temp folder from my environment in there:

TMPDIR=/var/folders/3H/3Hu3TTyjFtuvtN3D5tDUxU+++TI/Tmp/

So presumably these tasks are supposed to clean it up after they use it. But none of my attempts to use this task so far have succeeded, so I suspect the problem is that the folder does not get cleaned up in the event of an error. This folder needs to get cleaned up on all errors, or perhaps cleared at the start of a new run (that is probably going to turn out to be a more reliable fix).

I'll manually clear it and move on to the bug I was originally looking to reproduce.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Jonathan H. Wage added a comment - - edited

Hmm. When I look at Doctrine_Migration_Diff we implement a _cleanup() method that is for that purpose. Does it not clean up the directory properly?

Show
Jonathan H. Wage added a comment - - edited Hmm. When I look at Doctrine_Migration_Diff we implement a _cleanup() method that is for that purpose. Does it not clean up the directory properly?
Hide
Permalink
Tom Boutell added a comment -

[Peeks at source]

Looks like you do it last. If an exception is thrown it doesn't happen (I just verified that I have a full folder of classes left behind after an attempt to use it). Then you have a lingering problem confounding your attempts to try again.

Suggest you call _cleanup() at the start and the end. The first call won't take much time at all if the last run was a happy one. If it was an unhappy one you really need it.

  • * *

A much more minor issue:

I'm a little nervous about the fact that if I were running two schema diffs for two different projects at once, they would still collide, sharing the same tmp folder. I would suggest starting everything from TMP/doctrine-$pid where $pid = getpid() or something like that.

In pkToolkit we use SF_ROOT_DIR/data/pk_writable/tmp because that is safely project-specific.

Show
Tom Boutell added a comment - [Peeks at source] Looks like you do it last. If an exception is thrown it doesn't happen (I just verified that I have a full folder of classes left behind after an attempt to use it). Then you have a lingering problem confounding your attempts to try again. Suggest you call _cleanup() at the start and the end. The first call won't take much time at all if the last run was a happy one. If it was an unhappy one you really need it. * * A much more minor issue: I'm a little nervous about the fact that if I were running two schema diffs for two different projects at once, they would still collide, sharing the same tmp folder. I would suggest starting everything from TMP/doctrine-$pid where $pid = getpid() or something like that. In pkToolkit we use SF_ROOT_DIR/data/pk_writable/tmp because that is safely project-specific.
Hide
Permalink
Jonathan H. Wage added a comment -

Tom, as you have an environment setup with the problem exposed, could you prepare a patch that fixes the problems you are encountering?

Show
Jonathan H. Wage added a comment - Tom, as you have an environment setup with the problem exposed, could you prepare a patch that fixes the problems you are encountering?
Hide
Permalink
Jonathan H. Wage added a comment -

So I just want to confirm. We have two issues now. The _cleanup() method needs to be called when it starts. This patch should take care of that:

Index: lib/Doctrine/Migration/Diff.php
===================================================================
--- lib/Doctrine/Migration/Diff.php	(revision 6882)
+++ lib/Doctrine/Migration/Diff.php	(working copy)
@@ -93,6 +93,8 @@
      */
     public function generateChanges()
     {
+        $this->_cleanup();
+
         $from = $this->_generateModels(self::$_fromPrefix, $this->_from);
         $to = $this->_generateModels(self::$_toPrefix, $this->_to);

Now we have another issue where the paths could conflict. Can we open another ticket to work on that issue? Can you confirm that the above patch fixes this individual issue?

Show
Jonathan H. Wage added a comment - So I just want to confirm. We have two issues now. The _cleanup() method needs to be called when it starts. This patch should take care of that: Index: lib/Doctrine/Migration/Diff.php =================================================================== --- lib/Doctrine/Migration/Diff.php (revision 6882) +++ lib/Doctrine/Migration/Diff.php (working copy) @@ -93,6 +93,8 @@ */ public function generateChanges() { + $ this ->_cleanup(); + $from = $ this ->_generateModels(self::$_fromPrefix, $ this ->_from); $to = $ this ->_generateModels(self::$_toPrefix, $ this ->_to); Now we have another issue where the paths could conflict. Can we open another ticket to work on that issue? Can you confirm that the above patch fixes this individual issue?
Hide
Permalink
Tom Boutell added a comment -

Yes that is the correct patch, I just prepared the exact same patch and went back to my email to find the ticket and saw your comment.

I will open a separate ticket re: the potential clash between projects. It's certainly less probable but it doesn't make sense for them to contend for a single folder.

Thanks!

Show
Tom Boutell added a comment - Yes that is the correct patch, I just prepared the exact same patch and went back to my email to find the ticket and saw your comment. I will open a separate ticket re: the potential clash between projects. It's certainly less probable but it doesn't make sense for them to contend for a single folder. Thanks!
Hide
Permalink
Jonathan H. Wage added a comment -

Cool. Yes I think we can fix the other issue but in another revision/ticket. I will go ahead and commit that patch and close this ticket.

Thanks, Jon

Show
Jonathan H. Wage added a comment - Cool. Yes I think we can fix the other issue but in another revision/ticket. I will go ahead and commit that patch and close this ticket. Thanks, Jon

People

  • Assignee:
    Jonathan H. Wage
    Reporter:
    Tom Boutell
Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: