Doctrine 1
  1. Doctrine 1
  2. DC-333

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

    Details

    • Type: Bug Bug
    • Status: 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

        Hide
        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
        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
        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
        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
        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
        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
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: