Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow builders to be added through options array #236

Closed

Conversation

cleentfaar
Copy link

Like providers, it would be neat to have builders configurable through the $options argument:

$options = [
    'locale' => $locale,
    'logger' => $this->container->get('logger'),
    'providers' => [new CustomProvider()],
    'builders' => [new CustomBuilder()], // new option
];

I needed this to implement my own name flag (uses...) which allows me to update an already persisted fixture with new data:

  # default (dutch) article
  article_hello_world_nl:
    title: Hallo wereld!
    subtitle: Dit is je eerste artikel!
    body: Wat een lekker weertje vandaag, nietwaar?

  # english version of the article (requires same object to modify only a few properties)
  article_hello_world_en (local, uses article_hello_world_nl):
    title: Hello world!
    subtitle: This is your first article!
    body: What a lovely weather today, isn't it?
    translatableLocale: en

This approach (updating an existing entity) is required to work with the Translatable doctrine extension, as described here: https://github.com/Atlantic18/DoctrineExtensions/blob/master/doc/translatable.md#basic-usage-examples.

I won't go into the details of my builder as it's not really related to this change; I just hope you will be willing to merge this so others can make use of this as well.

@cleentfaar
Copy link
Author

As a side note:

I found quite some missing/incorrect phpdoc's in this library. Would you be willing to merge a PR that fixes a lot of them? I think it would really improve the self-explanatory aspect of the code and invite users to make use of more advanced functions your loader offers.

😄 Oh and thanks for making this library, I use it every day!

@tshelburne
Copy link
Collaborator

@cleentfaar You should always feel free to submit a PR to improve documentation - we really appreciate the support.

@Seldaek I'm kind of indifferent, but I feel like we should not open the doors to passing additional configuration options in this way. It seems to me we are already offering this level of configuration just by using the loader class directly, and Fixtures is exclusively for convenience. That said, if you'd rather open that door, fine by me.

@tshelburne
Copy link
Collaborator

@Seldaek ping

@Seldaek
Copy link
Member

Seldaek commented Sep 13, 2015

I don't know what to think to be honest, it seems like a small change but yes as you say the more explicit use is also possible and not so hard, I guess it's mostly a documentation problem..

@tshelburne
Copy link
Collaborator

Ok, I'm thinking we merge it since this is the current interface, but make a point of reducing the footprint of Fixtures for a future 3.0 release. There are already a few issues requesting additions to the interface, and I think we would be better served to reduce the options and fully document the approach to a "custom" circumstance. See #242 as an example.

@cleentfaar Please add documentation for this addition, and we'll go with it.

@theofidry
Copy link
Member

As it's a fairly old issue and requires a bit of work I'm closing this. Feel free to open a new rebased PR if you still wish to add it.

@theofidry theofidry closed this Sep 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants