Support alternative mapping formats

[K-Phoen]

This PR provides support for yaml mapping description (in addition to annotations).
Implementing XML support should be an easy task.

[K-Phoen]

Tests are on their way.

[K-Phoen]

Done. Waiting for a review :)

[K-Phoen]

Any feedback?

[quernelpak]

Do you give me an entity(yml) example please?

[K-Phoen]

Here it is:

Jedi\DomainBundle\Entity\Article:
  type:   entity
  table:  articles
  repositoryClass: Jedi\DomainBundle\Repository\ArticleRepository

  vich:
    uploadable:
      file:
        mapping:           article_pdf
        filename_property: file_name

  fields:
    id: 
      type:       integer
      id:         true
      generator:  {strategy: AUTO}

    title:
      type:       string
      length:     255 

    file_name:    
      type:       text

    slug:
      type:       string
      length:     255 
      gedmo:
        slug:     { fields: [title] }

    # ...

[K-Phoen]

@dustin10 so, what should I do to have this PR merged? :)

[ftassi]

HI @K-Phoen, It would be a really nice feature to have, alternative mappings is definitely something that I would put in the bundle.
That said I don't like to use dmr/drm to mix Vich mapping data and Doctrine mapping data. I would avoid drm/drm usage for two reasons:

• It depends on doctrine 2.3+ and I would like to keep this bundle working also for Symfony 2.0 with doctrine 2.2 installations
• There is no real needs to mix our mapping info with doctrine's.

If you think about it, Vich's mapping data are not so different from the validator annotations (@Assert/XXX) or the JmsSerializerBundle annotations (http://jmsyst.com/libs/serializer), I think we should do as they do: allowing extra mapping datas without mixing with doctrine.

What do you think about it?

If you don't have time to rewrite the pull request, or if don't want to, as a first step, in order to not throw away your work, I could merge it if dmr/dmr became a suggestion and not an hard dependency. This way, if you have doctrine 2.3+ you can install dmr/dmr and enable different mapping, but if you're working with doctrine 2.2 you can still use the bundle with annotations as it is working now.

In my opinion it would be better to rewrite this without dmr/dmr, allowing the user to specify a separate yml o xml mapping file for upload information, but if you want to keep dmr/dmr, at least, it has to be a soft dependency and not and hard one.

[K-Phoen]

I think it's nice to have feedback, especially when it's a relevant one! No need to merge this PR right now, I'll rewrite it to remove the dependency to dmr/dmr.

Thanks!

[K-Phoen]

@ftassi I modified the PR to use jms/metadata instead of dmr/dmr. The dependency to doctrine is now limited to the annotation reader (for the metadata part of the bundle of course).

Can you review it?

[ftassi]

you missed a comma here

[Gemorroj]

👍

[K-Phoen]

I'll try to add XML support soon.

[ftassi]

It make sense for me now, I guess that adding some documentation about this new feature we could merge it.

[K-Phoen]

Is this enough? https://github.com/dustin10/VichUploaderBundle/pull/159/files#diff-17

[ftassi]

Sorry, I've missed the updated doc file.

By the way, reading it I've thought that was kind of weird to have to define upload annotation within the entity YAML file. Metadata for uploadable fields aren't really related to doctrine, the bundle actually relies on doctrine events to trigger the upload, but it could work even without it.

Can't we put our metadata in a separate yaml file ? Wouldn't it be better ?

I haven't checked it yet, but I guess your code should work with a separate file right now if we just let the user to configure where he wants to store them and if the annotation driver gets the right file.

We should look for some pattern from other bundles, JmsSerializerBundle stores its metadata in a separate file, I think that's a good pattern to follow, it leads to a better decoupling for our bundle.

If we want to go for a separate file we should document where put the file and how to configure its position (if configurable).

[K-Phoen]

Ultimately this would be great.

As a matter of fact, I even think it's already possible. If you read the DependencyInjection/Configuration.php, DependencyInjection/VichUploaderExtension.php and Metadata/Driver/FileLocator.php files, you'll notice that even if I look by default in doctrine's configuration directories, you can specify your own locations. Though the expected YAML configuration is really coupled to doctrine's.

I think we should merge this PR as is and in the meantime work on a new version of this bundle - let's say 1.0 - with (at least) the following objectives:

• fixing the known bugs (delete_on_update don't works. #154 and [WIP] Refactor event system #106 for instance) ;
• decoupling the code from doctrine (which includes moving the configuration part into its own file).

What do you think?

[K-Phoen]

Hum. I don't know why I said that the yaml config was coupled to doctrine...

[ftassi]

@K-Phoen as you said reading metadata from a separate yaml file should be quite easy, maybe it's just a matter of updating the documentation and defining a new default directory for the yaml file.

If you agree that a separate file is the way to go, I'd like to update the PR before merging it. If we merge it as it is now, and users start to use this new feature they'll have to split their yaml file when we'll decouple the yamls.

[K-Phoen]

Done :)

The configuration files are searched in Resources/config/vich_uploader by default.

[ftassi]

I guess we can merge it now and get some feedback from users.