Skip to content

Loading…

DDC-218: @Version column is not reflected in resultColumnNames in StandardEntityPersister, generates E_NOTICE #2872

Closed
doctrinebot opened this Issue · 15 comments

1 participant

@doctrinebot

Jira issue originally created by user mzach:

Scenario: 2 classes with a "OneToMany" Bi-Directional Mapping

abstract class AbstractBase
{
    /**** 
     * @Id @Column(type="integer")
     * @GeneratedValue(strategy="AUTO")
     * @var integer
     */
    public $id;

    /****
     * @Version @Column(name="version", type="integer")
     * @var integer
     */
    public $version;
...
}

class Property extends AbstractBase
{
...
    /****
     * @todo used to have an "'orderBy' => 'order_number ASC' // default ordering for this field" back in 1.2.x
     * http://www.doctrine-project.org/documentation/manual/2_0/en/association-mapping#one-to-many,-bidirectional
     * @OneToMany(targetEntity="Webges\Core\Property\PropertyOption", mappedBy="Property")
     * @var \Webges\Core\Property\PropertyOption
     */
    public $Options;
}

class PropertyOption extends AbstractBase
{
...
    /****
     * http://www.doctrine-project.org/documentation/manual/2_0/en/association-mapping#one-to-many,-bidirectional
     * @ManyToOne(targetEntity="Webges\Core\Property\Property")
     * @JoinColumn(name="property_id", referencedColumnName="id")
     * @var \Webges\Core\Property\Property
     */
    public $Property;
}

When I load an instance of Property using DQL and lazy-load the PropertyOption objects, I get an E_NOTICE saying "Notice: Undefined index: version in Doctrine/ORM/Persisters/StandardEntityPersister.php on line 579"

After debugging the source I noticed that the MetaClass definitionfor PropertyOption is correct - the class is marked as "isVersioned", also the "version" column is contained in the "columnNames" and "fieldNames" mapping, but not in "resultColumnNames" - this is also true for some other fields contained in AbstractBase

Maybe this is desired behaviour and there is a fix to it which I might not see yet (clues appreciated!), but to me and my colleague this seems like a bug in the data mapping.

@doctrinebot

Comment created by romanb:

Is AbstractBase a mapped superclass (@MappedSuperclass) ?

@doctrinebot

Comment created by mzach:

No, it's not - just a simple "abstract class" containing some fields which should be passed on to the extending classes, like

  • id
  • version
  • createdByPersonId
  • createdAt
  • modifiedByPersonId
  • modifiedAt
  • deletedByPersonId
  • deletedAt

The idea is to have these fields passed from the abstract class rather than implementing each of it in the concrete class.
Interestingly, it works on the Property class but not on the PropertyOption class.

Let me know if you need further details

@doctrinebot

Comment created by romanb:

Well, it really should be a @MappedSuperclass. Otherwise all these properties on that class should be ignored altogether, and they are.
The issue is just that these properties are inherited through regular PHP inheritance and thus it appears like this works. So there is another bug here. If there is neither @Entity, nor @MappedSuperclass on a class then this class should be ignored by doctrine altogether. Currently the inherited fields "slip through".

Anyway, I dont think this changes something but you should make AbstractBase a mapped superclass. Thats exactly what they are there for: to provide mapping information and persistent fields that are common to all subtypes but the mapper superclass itself is not mapped.

Please tell me if applying @MappedSuperclass makes a difference.

I will try to reproduce it in our test suite.

Thanks.

PS: I assume the fields are not public in the real app, right? If so, be aware that this can break lazy-loading. Always use private or protected.

@doctrinebot

Comment created by romanb:

For information on mapped superclasses, see: http://www.doctrine-project.org/documentation/manual/2_0/en/inheritance-mapping:mapped-superclasses

@doctrinebot

Comment created by mzach:

Changed the class to @MappedSuperclass, but this did not change anything

For testing purpose properties are currently public since we're just starting to migrate from 1.2.x to 2.0.x last Friday.

Anyway, is there a problem if the @MappedSuperclass extends another @MappedSuperclass ?

Like:


/****
 * @MappedSuperclass
 */
abstract class AnotherSuperClass
{
    /**** @Column(name="some_column", type="integer")
    protected $someColumn;
}

/****
 * @MappedSuperclass
 */
abstract SuperClass extends AnotherSuperClass
{
    /**** @Column(name="new_field", type="string")
    protected $newField;
}

/****
 * @Entity
 */
class ConcreteClass extends SuperClass
{
}
@doctrinebot

Comment created by mzach:

As a side note: using ZendFramework 1.9.6 with Doctrine2.0.0-ALPHA3 with APC enabled and the uppermost @MappedSuperclass lies in the outer namespace (\Webges\Core) whereas the concrete classes are in \Webges\Core\Property

@doctrinebot

Comment created by romanb:

I have attached a test case that works currently (no warnings/notices/...)

Can you tell me how to modify this test so that it reveals your issue?

If you want to run the test yourself and you have the tests checked out via svn, just drop the attached file into the tests/Doctrine/Tests/ORM/Functional/Ticket/ directory and then from the command line, within the tests folder, run "phpunit Doctrine/Tests/ORM/Functional/Ticket/DDC218Test".

@doctrinebot

Comment created by romanb:

Of course, I am testing against trunk.

@doctrinebot

Comment created by romanb:

Also note that when you're using APC as the metadata cache during development that you may need to flush the cache in order for changes to take effect.

During development its usually the most convenient to work without a metadata cache.

Note on the caches: In a production environment, APC is the best choice for metadata & query cache, even if you scale horizontally across multiple servers since the metadata is not related to users (its not like session data) and is the same on each server thus each PHP server can have its own APC metadata cache, which only occupies a few MB of RAM with a few hundred mapped classes. Memcache is a good alternative but slower since it involves network overhead.

@doctrinebot

Comment created by mzach:

I know, also tried the ArrayCache instead of ApcCache, but this does not affect the outcome.

Going to check your test and then modify it to reflect our current setup.

@doctrinebot

Comment created by mzach:

I've re-created the scenario within the testcase and figured two things out:

  • first: version-Problem was fixed in trunk, exists in -ALPHA3
  • second: it's not necessary any more to have an whateverId column in the entity itself if an relation $whatever exists, which we had during a manual update

So from my side this issue can be closed, just need to upgrade to /trunk.

Thank you

@doctrinebot

Comment created by romanb:

Ok, thanks. I mark this as fixed for ALPHA4 then.

And your observation about the foreign keys is correct. There is no need to have foreign keys in the object model. They're details of the relational database. Your objects have object references, not foreign keys, obviously. This is an important aspect of decoupling the objects from the concrete data store where they are persisted. For example, if you later want to store the same objects in another storage like CouchDB, the foreign keys have absolutely no meaning. Sometimes, unfortunately, it is still necessary to map foreign keys into object fields currently when using some complex composite keys. But when you stay away from composite keys (except in a pure many-many association table that has no corresponding entity, in which case you get the composite key generated by doctrine anyway) then you're all fine.

@doctrinebot

Issue was closed with resolution "Fixed"

@doctrinebot doctrinebot added this to the 2.0-ALPHA4 milestone
@doctrinebot doctrinebot closed this
@doctrinebot doctrinebot added the Bug label
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.