fsi-open / doctrine-extensions Goto Github PK

example to reproduce:

/**
 * @Entity
 * @InheritanceType("SINGLE_TABLE")
 */
class Person
{
    // ...
}

/**
 * @Entity
 */
class Employee extends Person
{
    /**
     * @ORM\Column(type="string", length=255, nullable=true)
     * @FSi\Uploadable(targetField="file")
     */
    private $filePath;

    private $file;
}

Exception:

Mapping "Uploadable" in property "filePath" of class "Person" has 
"targetField" set to "file", which doesn't exist.

It would be very helpful if all files from bundle extends from \SplFileInfo. If someone use this with symfony (doctrine-extensions-bundle) we could add return type hint in methods. Symfony already extends from it and \SplfileInfo doesnt need the file to exist:

SplFileInfo::__construct( string $file_name )
Creates a new SplFileInfo object for the file_name specified. The file does not need to exist, or be readable.

If we have private id property in base class like this:

/** @MappedSuperclass */
class Upper {
    /** @Id */
    private $id;
    public getId() {return this->id;}
}

/** @Entity */
class Lower extends Upper {
}

ReflectionException : Property Lower::$id does not exist

it should be changed here and probably in both cases (Proxy and object) this should work:

$em->getClassMetadata(get_class($object))->getIdentifierValues($object);

Case:

1. Entity A has translations in Entity AT.
2. Entity B extends A through joined inheritance and has translations in Entity BT.
3. Translated fields for B are shared between AT and BT.
4. Persisting and instance of B, which only has translated fields stored in the AT fails. That is because the empty fields stored in BT cause the whole translation to be removed and you cannot have an AT row without a corresponding BT one. Example below:

class A
{
   /**
    * @var string
    **/
   public $locale;

   /**
    * @var string|null
    **/
   public $fieldA;

   /**
    * @var AT[]
    **/
   public $aTranslations;
}

class B extends A 
{
   /**
    * @var string|null
    **/
   public $fieldB;

   /**
    * @var BT[]
    **/
   public $bTranslations;
}

class AT 
{
   /**
    * @var string
    **/
   public $locale;

   /**
    * @var string|null
    **/
   public $fieldA;

   /**
    * @var A
    **/
   public $translatedEntity;
}

class BT 
{
   /**
    * @var string
    **/
   public $locale;

   /**
    * @var string|null
    **/
   public $fieldB;

   /**
    * @var B
    **/
   public $translatedEntity;
}

$instanceOfB = new B();
$instanceOfB->locale = 'en';
$instanceOfB->fieldA = 'value';
// Since `fieldB` is null, the empty BT translation is going to be removed.
// This means AT is not created or removed.
$instanceOfB->fieldB = 'value';
// Only now both translations will be properly created, because the `fieldB` field is not `null`

Issues.

Since they are fairly simple and only used in this package, it would be best to simply extract the used functionality and deprecate the standalone packages.

Conitnuation of #61

Missing cases:

• mappedBy, field, targetEntity with empty values.
• No locale in translation entity (YAML/XML).
• Figure out a better way to get doctrine mapping XSD file path.
• Better exception message during schema validation - currently the real error is masked.

Current implementation assumes possibility of multiple adapters (ORM/ODM etc.), which has never (and probably will never) be implemented.

Assuming you have an entity with a relation handled by it's translation, it will fail during save if the value for the field of that relation is null. Of course initially you need to pass an array or an ArrayCollection instance to the file in the constructor, which solves the problem during creation. The issue arises in a situation when:

1. There are no objects in the said relation.
2. The translated object is fetched via the entity manager (so the constructor is omitted).
3. You edit the object through a form where the field for that relation does not exist.

This will result in a null value being copied to the translation class's field. To get around this, you need to add a lifecycle callback (like postLoad) to the translated entity, that will properly set the value.

This is not a huge issue and it does not require a hack to get around, but perhaps we could check for a null value and if the field to which it is copied is a relation, set an empty array instead?

It removes the file, but the entity's field is still set to the old value.

When trying to remove entity that have filekey but file not longer exists (somebody remove it manualy) exception is throwed but entity is removed.
I think it would be enough to check if file exists before removing it.

JarekW created branch findby at fsi-open/doctrine-extensions 10 minutes ago

Any good reason for that? New branch means also new version at packagist.
@chives maybe some user permissions should be revoked only to read?

... that is set per field

@Translatable\Translatable(mappedBy="translations", copyFromDefault=true)

This is useful when working with translatable collections. When collection is translatable you must have at least one element in collection to not show elements from default translation element. There's no way to have nothing in specific language when default translation have some elements.

i'm aware that this might introduce some complications with something that depend on default translations

Current implementation requires public access for modifying translated fields. This is troublesome in a situation, where you would like to only have read access to entity properties. We could add an option of setting the translation via a specified method, which would inject a populated translation object for the requested locale.

When you have a sluggable entity and create a new translation in a language different than the default one, it pretty much always creates the slug based on the value from the default translation. This creates a duplicate slug in a wrong language, with added suffix (like -1). If you change the slug-related field afterwards, it will regenerate the slug in the proper locale.

There is something wrong with "generateNewKey" method in UploadableListener

PHP Notice:  Undefined offset: 3 in
/www/fsi-open-demo/vendor/fsi/doctrine-extensions/lib/FSi/DoctrineExtensions/Uploadable/UploadableListener.php
on line 566

I'm getting this errer very often.

If class has file field but doesn't has setter for it, for example file is set in constructor, then NoSuchPropertyException is thrown down here

It should be done as doctrine do this - through reflection.

revert #8

Moreover getters should also change. Current implementation with PropertyAccessor block developers from using php7 return types declaration.

doctrine/doctrine2 since 2.5 dispatches postLoad event after all associations are initialized so the custom hydrator is not needed anymore