Doctrine 1
  1. Doctrine 1
  2. DC-676

Validation exception thrown only if internal nesting level == 1

    Details

    • Type: Bug Bug
    • Status: Reopened
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 1.2.2
    • Fix Version/s: None
    • Component/s: Validators
    • Labels:
      None
    • Environment:
      PHP 5.3.1, Mac OS X 10.6

      Description

      I wonder why Validation exception is thrown only if internal nesting level of transaction is 1 and the manual nesting level is not considered.

      Line 262 - lib/Doctrine/Transaction.php:

      if ($this->_internalNestingLevel == 1) {
          $tmp = $this->invalid;
          $this->invalid = array();
          throw new Doctrine_Validator_Exception($tmp);
      }
      

      I have a large database with a lot of tables and relationships. At some case the validation exception is not thrown if the validation fails.

      If I add $this->_nestingLevel into this condition:

      if ($this->_internalNestingLevel == 1 || $this->_nestingLevel == 1) {
          $tmp = $this->invalid;
          $this->invalid = array();
          throw new Doctrine_Validator_Exception($tmp);
      }
      

      Then the validation exception is thrown as excepted.

      I don't know why you ignore here the nesting level.

      It would be great if you could explain me more about these lines.

      I ran the unit test with this modification and got the same result.

        Activity

        Hide
        Fabian Spillner added a comment -

        Additional information: On doctrine 1.1.x the exception is thrown without this modification.

        Show
        Fabian Spillner added a comment - Additional information: On doctrine 1.1.x the exception is thrown without this modification.
        Hide
        Fabian Spillner added a comment -

        It seems, the problem is the counter of internal nesting level. I output these member variables:

        Doctrine 1.1.2

        Doctrine_Transaction::commit invalid is not empty - internal nesting level: 1 nesting level: 2
        
        ok 1 - Article cannot be created if empty: Doctrine_Validator_Exception: Validation failed in class Article
        
          1 field had validation error:
        
            * 1 validator failed on slug_title (notnull)
        

        Doctrine 1.2.2

        Doctrine_Transaction::commit invalid is not empty - internal nesting level: 0 nesting level: 1
        
        not ok 1 - Empty Artcile could be created!
        
        Show
        Fabian Spillner added a comment - It seems, the problem is the counter of internal nesting level. I output these member variables: Doctrine 1.1.2 Doctrine_Transaction::commit invalid is not empty - internal nesting level: 1 nesting level: 2 ok 1 - Article cannot be created if empty: Doctrine_Validator_Exception: Validation failed in class Article 1 field had validation error: * 1 validator failed on slug_title (notnull) Doctrine 1.2.2 Doctrine_Transaction::commit invalid is not empty - internal nesting level: 0 nesting level: 1 not ok 1 - Empty Artcile could be created!
        Hide
        Fabian Spillner added a comment -

        Yeah! I found the reason:

        I attached a template (listener) into my model which validates the body and this method call:

        // ...
        if ($obj->getAuthor()->getAge())  # this creates new empty Author object
        {
          // ...
        }
        // ...
        

        On Doctrine 1.1, the method saveGraph() of class UnitOfWork executes first saveRelatedLocalKeys and then the hooks methods (preSave, etc.) and my code works!

        On Doctrine 1.2, the method saveGraph() call *first* the hooks method and then the saveRelatedLocalKeys which save the empty author object and then destroy the internal nesting level
        and the validation exception is not thrown.

        Workaround for my listener:

        // ...
        if ($obj->relatedExists("Author") && $obj->getAuthor()->getAge())  # relatedExists() needed to avoid create empty Author object
        {
          // ...
        }
        // ...
        

        What do you think: It's a bug or a feature?

        Show
        Fabian Spillner added a comment - Yeah! I found the reason: I attached a template (listener) into my model which validates the body and this method call: // ... if ($obj->getAuthor()->getAge()) # this creates new empty Author object { // ... } // ... On Doctrine 1.1, the method saveGraph() of class UnitOfWork executes first saveRelatedLocalKeys and then the hooks methods (preSave, etc.) and my code works! On Doctrine 1.2, the method saveGraph() call * first * the hooks method and then the saveRelatedLocalKeys which save the empty author object and then destroy the internal nesting level and the validation exception is not thrown. Workaround for my listener: // ... if ($obj->relatedExists("Author") && $obj->getAuthor()->getAge()) # relatedExists() needed to avoid create empty Author object { // ... } // ... What do you think: It's a bug or a feature?
        Hide
        Fabian Spillner added a comment -

        A test case attached to reproduce the bug.

        The test passes if you remove the custom save() method of Ticket_DC676_Author class.

        The problem: default "foobar" modifies the Author and is modified. Doctrine tries to save it,
        but because there is more than two transactions, the validation exception is not thrown for Article object.

        Show
        Fabian Spillner added a comment - A test case attached to reproduce the bug. The test passes if you remove the custom save() method of Ticket_DC676_Author class. The problem: default "foobar" modifies the Author and is modified. Doctrine tries to save it, but because there is more than two transactions, the validation exception is not thrown for Article object.
        Hide
        Jonathan H. Wage added a comment -

        Something that really helps with determining what we should do is to find the exact changeset that causes the behavior. If we can look at what code was changed, we can then understand better what to do.

        Show
        Jonathan H. Wage added a comment - Something that really helps with determining what we should do is to find the exact changeset that causes the behavior. If we can look at what code was changed, we can then understand better what to do.
        Hide
        Fabian Spillner added a comment - - edited

        This changeset affects the problem:
        http://trac.doctrine-project.org/changeset/6126/branches/1.2/lib/Doctrine/Connection/UnitOfWork.php

        But I really don't like this logic how the validation exception is handled. I would seperate these things:

        The method validate() should throw exception, and the method commit() should commit only if model is valid and nothing more.

        1. go recursive into the model and call the validate() method

        2. if not valid, exception is thrown

        3. elseif ok, commit is called

        PS. Sorry for late answer: I had a lot of work ...

        Show
        Fabian Spillner added a comment - - edited This changeset affects the problem: http://trac.doctrine-project.org/changeset/6126/branches/1.2/lib/Doctrine/Connection/UnitOfWork.php But I really don't like this logic how the validation exception is handled. I would seperate these things: The method validate() should throw exception, and the method commit() should commit only if model is valid and nothing more. 1. go recursive into the model and call the validate() method 2. if not valid, exception is thrown 3. elseif ok, commit is called PS. Sorry for late answer: I had a lot of work ...
        Hide
        Jonathan H. Wage added a comment -

        Do you see something with that commit that could be changed that fixes your issue?

        Show
        Jonathan H. Wage added a comment - Do you see something with that commit that could be changed that fixes your issue?
        Hide
        Jonathan H. Wage added a comment -

        Whoops. This change is causing some problems. It causes lots of tests to fail in our test suite. Reverting for now. Can you try your change against our test suite and see if you can determine anything? Thanks, Jon

        Show
        Jonathan H. Wage added a comment - Whoops. This change is causing some problems. It causes lots of tests to fail in our test suite. Reverting for now. Can you try your change against our test suite and see if you can determine anything? Thanks, Jon

          People

          • Assignee:
            Jonathan H. Wage
            Reporter:
            Fabian Spillner
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated: