Add a way to load fixtures from a specific bundle #130

Merged
merged 5 commits into from Apr 6, 2012

Conversation

Projects
None yet
4 participants
@Palleas

Palleas commented Apr 4, 2012

Today I wanted to load a set of fixtures from a specific bundle. With this PR you'll be able to do :

$ app/console propel:fixtures:load @MySuperBundle

What do you think?

@havvg

This comment has been minimized.

Show comment
Hide comment
@havvg

havvg Apr 4, 2012

Member

I like the idea, but don't like the implementation, could you re-factor it into using the FileLocator?

Member

havvg commented Apr 4, 2012

I like the idea, but don't like the implementation, could you re-factor it into using the FileLocator?

@willdurand

This comment has been minimized.

Show comment
Hide comment
@willdurand

willdurand Apr 4, 2012

Member

👍

Member

willdurand commented Apr 4, 2012

👍

@hhamon

This comment has been minimized.

Show comment
Hide comment
@hhamon

hhamon Apr 5, 2012

Member

I don't like the @ before the name of the template. Can we just tell the bundle name instead?

Member

hhamon commented Apr 5, 2012

I don't like the @ before the name of the template. Can we just tell the bundle name instead?

@willdurand

This comment has been minimized.

Show comment
Hide comment
@willdurand

willdurand Apr 5, 2012

Member

@hhamon isn't a kind of convention used in other bundles?

Member

willdurand commented Apr 5, 2012

@hhamon isn't a kind of convention used in other bundles?

@havvg

This comment has been minimized.

Show comment
Hide comment
@havvg

havvg Apr 5, 2012

Member

Is the actual path fixed, or do we want to specify a complete path to the fixtures directory.
Something like $ app/console propel:fixtures:load @AcmeBlogBundle/Resources/fixtures/demo? This would also allow to alter the fixtures of a bundle using bundle inheritance. The default fixtures directory will be unchanged, meaning app/propel/fixtures.

Member

havvg commented Apr 5, 2012

Is the actual path fixed, or do we want to specify a complete path to the fixtures directory.
Something like $ app/console propel:fixtures:load @AcmeBlogBundle/Resources/fixtures/demo? This would also allow to alter the fixtures of a bundle using bundle inheritance. The default fixtures directory will be unchanged, meaning app/propel/fixtures.

@hhamon

This comment has been minimized.

Show comment
Hide comment
@hhamon

hhamon Apr 5, 2012

Member

I have never seen the @ notation from a CLI command in Symfony...

Member

hhamon commented Apr 5, 2012

I have never seen the @ notation from a CLI command in Symfony...

@havvg

This comment has been minimized.

Show comment
Hide comment
@havvg

havvg Apr 5, 2012

Member

BehatBundle for example does it, to run features from a specific bundle.

Member

havvg commented Apr 5, 2012

BehatBundle for example does it, to run features from a specific bundle.

@Palleas

This comment has been minimized.

Show comment
Hide comment
@Palleas

Palleas Apr 5, 2012

Yep, the cool thing with Behat commands is running very specifics scenarios with :line (for example) but I'm not sure we need that.
I'm ok to use the file locator (even if I'm not sure why, because I'm only retrieving a path so it works just like if we'd used the -d option), but @havvg can you tell me more about what you don't like in this implementation?
I wanted to keep it very simple and directly look for Resources/fixtures folder, "convention over configuration" or something like that ;-)

Palleas commented Apr 5, 2012

Yep, the cool thing with Behat commands is running very specifics scenarios with :line (for example) but I'm not sure we need that.
I'm ok to use the file locator (even if I'm not sure why, because I'm only retrieving a path so it works just like if we'd used the -d option), but @havvg can you tell me more about what you don't like in this implementation?
I wanted to keep it very simple and directly look for Resources/fixtures folder, "convention over configuration" or something like that ;-)

@willdurand

This comment has been minimized.

Show comment
Hide comment
@willdurand

willdurand Apr 5, 2012

Member

Yeah, I like the @ notation, just update your PR using the FileLocator as @havvg said, and it will be ok.

Member

willdurand commented Apr 5, 2012

Yeah, I like the @ notation, just update your PR using the FileLocator as @havvg said, and it will be ok.

@Palleas

This comment has been minimized.

Show comment
Hide comment
@Palleas

Palleas Apr 5, 2012

I'm sorry but I'm not sure I see what you mean when you want me to use the FileLocator, as all I'm doing is retrieving a path. You want to get rid of the absoluteFixturesPath property ?

Palleas commented Apr 5, 2012

I'm sorry but I'm not sure I see what you mean when you want me to use the FileLocator, as all I'm doing is retrieving a path. You want to get rid of the absoluteFixturesPath property ?

@havvg

This comment has been minimized.

Show comment
Hide comment
@havvg

havvg Apr 5, 2012

Member

There is a 'file_locator' service, which should be used to retrieve files/directories with a notation like you suggested, but the complete path. For example $this->container->get('file_locator')->locate('@AcmeBlogBundle/Resources/fixtures/demo/posts.yml'); which will yield the filename with bundle inheritance and other changes/customizations in mind. See https://github.com/propelorm/PropelBundle/blob/master/Command/AbstractPropelCommand.php#L234 for an example.

Member

havvg commented Apr 5, 2012

There is a 'file_locator' service, which should be used to retrieve files/directories with a notation like you suggested, but the complete path. For example $this->container->get('file_locator')->locate('@AcmeBlogBundle/Resources/fixtures/demo/posts.yml'); which will yield the filename with bundle inheritance and other changes/customizations in mind. See https://github.com/propelorm/PropelBundle/blob/master/Command/AbstractPropelCommand.php#L234 for an example.

@hhamon

This comment has been minimized.

Show comment
Hide comment
@hhamon

hhamon Apr 5, 2012

Member

For your information, the doctrine:generate:entities command does not take the @ notation for specifying a bundle name.

Member

hhamon commented Apr 5, 2012

For your information, the doctrine:generate:entities command does not take the @ notation for specifying a bundle name.

@Palleas

This comment has been minimized.

Show comment
Hide comment
@Palleas

Palleas Apr 6, 2012

I can remove it, I only had Behat in mind when I wrote this so...
I'll update this PR this afternoon, it's almost done :)

Palleas commented Apr 6, 2012

I can remove it, I only had Behat in mind when I wrote this so...
I'll update this PR this afternoon, it's almost done :)

Romain Pouclet added some commits Apr 6, 2012

@Palleas

This comment has been minimized.

Show comment
Hide comment
@Palleas

Palleas Apr 6, 2012

Updated :)

Palleas commented Apr 6, 2012

Updated :)

@willdurand

View changes

Command/FixturesLoadCommand.php
+ $files = $finder->in(null !== $in ? $in : $this->absoluteFixturesPath);
+
+ if (null === $this->bundle) {
+

This comment has been minimized.

@willdurand

willdurand Apr 6, 2012

Member

You should remove this blank line

@willdurand

willdurand Apr 6, 2012

Member

You should remove this blank line

This comment has been minimized.

@Palleas

Palleas Apr 6, 2012

I thought it was in the Symfony2's CS?

@Palleas

Palleas Apr 6, 2012

I thought it was in the Symfony2's CS?

This comment has been minimized.

@willdurand

willdurand Apr 6, 2012

Member

yes it is, but just if you have code before I think..

@willdurand

willdurand Apr 6, 2012

Member

yes it is, but just if you have code before I think..

This comment has been minimized.

@willdurand

willdurand Apr 6, 2012

Member

See the first code example: http://symfony.com/doc/current/contributing/code/standards.html the first return in the transform method doesn't have a blank line before.

@willdurand

willdurand Apr 6, 2012

Member

See the first code example: http://symfony.com/doc/current/contributing/code/standards.html the first return in the transform method doesn't have a blank line before.

This comment has been minimized.

@Palleas

Palleas Apr 6, 2012

Indeed, fixed!

@Palleas

Palleas Apr 6, 2012

Indeed, fixed!

@willdurand

View changes

Command/FixturesLoadCommand.php
+ $finalFixtureFiles = array();
+
+ foreach ($files as $file) {
+ $fixtureFilePath = str_replace($this->bundle->getPath(). DIRECTORY_SEPARATOR . 'Resources' . DIRECTORY_SEPARATOR . 'fixtures' . DIRECTORY_SEPARATOR, '', $file->getRealPath());

This comment has been minimized.

@willdurand

willdurand Apr 6, 2012

Member

You could probably avoid to write the same path twice (see line 131)

@willdurand

willdurand Apr 6, 2012

Member

You could probably avoid to write the same path twice (see line 131)

This comment has been minimized.

This comment has been minimized.

@willdurand

willdurand Apr 6, 2012

Member

perfect

@willdurand

willdurand Apr 6, 2012

Member

perfect

willdurand added a commit that referenced this pull request Apr 6, 2012

Merge pull request #130 from Palleas/load_from_bundle
Add a way to load fixtures from a specific bundle

@willdurand willdurand merged commit cfb7873 into propelorm:master Apr 6, 2012

@willdurand

This comment has been minimized.

Show comment
Hide comment
@willdurand

willdurand Apr 6, 2012

Member

Thanks!

Member

willdurand commented Apr 6, 2012

Thanks!

@Palleas

This comment has been minimized.

Show comment
Hide comment
@Palleas

Palleas Apr 6, 2012

Hiii, my first contribution to Propel \o/

Palleas commented Apr 6, 2012

Hiii, my first contribution to Propel \o/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment