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

Yii2 faker integration #1743

Merged
merged 13 commits into from Jan 3, 2014
Merged

Yii2 faker integration #1743

merged 13 commits into from Jan 3, 2014

Conversation

@Ragazzo
Copy link
Contributor

@Ragazzo Ragazzo commented Jan 2, 2014

Faker integration for Yii2. Ready for review. Sorry for big number of commits, but almost all of them about README.md, because of it is very important to have good docs.
Related with this PR - #1527

@Ragazzo
Copy link
Contributor Author

@Ragazzo Ragazzo commented Jan 2, 2014

Example:

#template
<?php

use yii\helpers\Security;

return [
    'name' => 'firstName',
    'phone' => 'phoneNumber',
    'city' => 'city',
    'password' => function ($fixture, $faker, $index) {
        $fixture['password'] = Security::generatePasswordHash('password_' . $index);
        return $fixture;
    },
    'auth_key' => function ($fixture, $faker, $index) {
        $fixture['auth_key'] = Security::generateRandomKey();
        return $fixture;
    },
];

in english language:

<?php

return [
    [
        'name' => 'Ashlynn',
        'phone' => '+80(9)6646157065',
        'city' => 'South Alda',
        'password' => '$2y$13$skQAkV9R4jT7hIHLxbokG.Sa69zUDD72z7l.HRfwpaI88j0If0/D.',
        'auth_key' => 'Vnl2xrBsbeZtyFaW5Io1JOa1kNY3SLmc',
    ],
    [
        'name' => 'Mason',
        'phone' => '(136)568-9489',
        'city' => 'Port Michelle',
        'password' => '$2y$13$W17HXn.BvIc.u6/epVNpPOiDS9M6XrtKO2PoTFf/uvWAVB/V6UA06',
        'auth_key' => 'EOuFgd9T107KbAjX57NTs5h8JgsImmen',
    ],
    [
        'name' => 'Saige',
        'phone' => '1-786-316-4966x37346',
        'city' => 'Sammietown',
        'password' => '$2y$13$HYyIIMjF1yZIS/25qHtsPOXjZFV719HGjPmlAEbysBBNYH8LcXcw.',
        'auth_key' => 'YwiSvLdUNeImPAmGBtuY4f_0tJErKj4Z',
    ],
];

in russian

<?php

return [
    [
        'name' => 'Валериан',
        'phone' => '+7 (922) 186-3304',
        'city' => 'Луховицы',
        'password' => '$2y$13$Oajf8L1aSZer2hs5.42eLOMUsF1BZHLVoZyRyx3i2cg6qHiG0ztfO',
        'auth_key' => '060yversO629mK-ucq6thrE2kWwmY5k9',
    ],
    [
        'name' => 'Богдан',
        'phone' => '+7 (922) 592-2549',
        'city' => 'Дорохово',
        'password' => '$2y$13$bEr2n/eiPOpDCUkVjgw11.WAG0YRORuawKDCk1LQTA9sVRyeaoqam',
        'auth_key' => 'eBMDswP0ICk7pzCOn-nDKSF4UvFAdVV_',
    ],
    [
        'name' => 'Яков',
        'phone' => '(812) 603-97-06',
        'city' => 'Шаховская',
        'password' => '$2y$13$mTFyq23q47SWKVtL20KpJuiS2/gRluyHPUEUjs8w7mCX.xS/3BPxy',
        'auth_key' => 'jBukRMkxefGOEyz2dKt3wgPu1wcSZlZ7',
    ],
];
* This command manage fixtures creations based on given template.
*
* Fixtures are one of the important paths in unit testing. To speed up developers
* work this fixtures can be generated automatically, based on prepared template.

This comment has been minimized.

@samdark

samdark Jan 2, 2014
Member

this -> these

* ~~~
*
* If you use callback as a attribute value, then it will be called as shown with three parameters:
* * $fixture - current fixture array.

This comment has been minimized.

@samdark

samdark Jan 2, 2014
Member

Better to use - as bullets here since * looks weird in phpdoc.

* More info in [Faker](https://github.com/fzaninotto/Faker.) library docs.
* @var array
*/
public $providers = array();

This comment has been minimized.

@samdark

samdark Jan 2, 2014
Member

Short array syntax should be used.

$templatePath = Yii::getAlias($this->templatePath);
$fixturesPath = Yii::getAlias($this->fixturesPath);

if ($this->needToGenerateAll($file))

This comment has been minimized.

@samdark

samdark Jan 2, 2014
Member

We should use { even for one liners now.

$files = FileHelper::findFiles($templatePath, ['only' => [$file.'.php']]);

foreach ($files as $templateFile)
{

This comment has been minimized.

@samdark

samdark Jan 2, 2014
Member

Should be on the same line as foreach.

{
if (is_null($this->_generator))
{
#replacing - on _ because Faker support only en_US format and not intl

This comment has been minimized.

@samdark

samdark Jan 2, 2014
Member

Better to use //

*/
public function needToGenerateAll($file)
{
return $file == self::GENERATE_ALL;

This comment has been minimized.

@samdark

samdark Jan 2, 2014
Member

Should it be a separate method? If yes, should it be public?

This comment has been minimized.

@Ragazzo

Ragazzo Jan 2, 2014
Author Contributor

yes, i think it should, public is enough here, because it could be overridden and almost all Yii2/Yii1 code use private/public.

*/
public function getTemplate($file)
{
$template = require($file);

This comment has been minimized.

@samdark

samdark Jan 2, 2014
Member

Currently it can't be called twice because of require w/o once. Can it be an issue?

This comment has been minimized.

@Ragazzo

Ragazzo Jan 2, 2014
Author Contributor

require calling each time, what exactly you are referring to? Tested it with tbl_user, tbl_profile templates, works fine with php yii faker all_fixtures and by single one.

This comment has been minimized.

@samdark
Copy link
Member

@samdark samdark commented Jan 2, 2014

Overall looks fine except minor code style issues. Haven't actually tried it though.

@Ragazzo
Copy link
Contributor Author

@Ragazzo Ragazzo commented Jan 2, 2014

Fixed. Also added check for filename when generating.

@samdark
Copy link
Member

@samdark samdark commented Jan 2, 2014

@yiisoft/core-developers need additional review.

* generate fixtures template files, according to the given format:
*
* ~~~
* #users.php file under $templatePath

This comment has been minimized.

@qiangxue

qiangxue Jan 2, 2014
Member

Please use // for inline comments. # is never used in any other core code.

/**
* Generates fixtures and fill them with Faker data.
* @param string $file filename for the table template. You can generate all fixtures for all tables
* by specifiyng keyword "all_fixtures" as filename.

This comment has been minimized.

@qiangxue

qiangxue Jan 2, 2014
Member

How about using * to stand for generating fixtures for all tables?
The command should also prompt for confirmation of generating fixtures.

* @param type $fixtures
* @return string exported fixtures format
*/
public function getExportedFormat($fixtures)

This comment has been minimized.

@qiangxue

qiangxue Jan 2, 2014
Member

How about simply export()?
getExportedFormat() doesn't sound right to me. It sounds like returning a format, but then "exported format" doesn't make much sense.

This comment has been minimized.

@Ragazzo

Ragazzo Jan 2, 2014
Author Contributor

var_export exports in old 5.3 arrays format. or what export() command do you mean?

This comment has been minimized.

@qiangxue

qiangxue Jan 2, 2014
Member

I'm talking about renaming the method. It could be exportFixtures().

This comment has been minimized.

@Ragazzo

Ragazzo Jan 2, 2014
Author Contributor

ok.

@Ragazzo
Copy link
Contributor Author

@Ragazzo Ragazzo commented Jan 2, 2014

@qiangxue how can i specify input value as array? for example i want to php yii faker users,profile,some_other_table and want it to be an array in $file parameter. Is it possible in Yii2? In Yii1 it was possible afaik.

@qiangxue
Copy link
Member

@qiangxue qiangxue commented Jan 2, 2014

This is not supported in 2.0. You need to split the input string by yourself.

@Ragazzo
Copy link
Contributor Author

@Ragazzo Ragazzo commented Jan 2, 2014

Ok, but i think it should be added.

@qiangxue
Copy link
Member

@qiangxue qiangxue commented Jan 2, 2014

What syntax would you propose to support array input?

@Ragazzo
Copy link
Contributor Author

@Ragazzo Ragazzo commented Jan 2, 2014

Fixed. Generation confirmation message added. Ready for review.

@Ragazzo
Copy link
Contributor Author

@Ragazzo Ragazzo commented Jan 2, 2014

What syntax would you propose to support array input?

as always something1,something2,something3. I dont think that user input will always contain , as not specific case. But if does, we can offer public $delimiter per console controller, that will explode() to array by that delimiter.

@Ragazzo
Copy link
Contributor Author

@Ragazzo Ragazzo commented Jan 2, 2014

Also, @qiangxue i wanted to support "*" as for all fixtures generating, but on my Ubuntu 13.10 basic application template php yii faker * yield $file parameter as assets. I think it could be some bug or whatever, thats why i stayed on all_fixtures.

@Ragazzo
Copy link
Contributor Author

@Ragazzo Ragazzo commented Jan 2, 2014

Done, ready for review.

@Ragazzo
Copy link
Contributor Author

@Ragazzo Ragazzo commented Jan 2, 2014

Attaching screenshots.
fixture_faker_screenshot
faker_fixtures_1

@Ragazzo
Copy link
Contributor Author

@Ragazzo Ragazzo commented Jan 2, 2014

Travis failed, not sure if it is my issue, because some problems with db.

@samdark
Copy link
Member

@samdark samdark commented Jan 2, 2014

That's not your issue. Just travis glitches.

@Ragazzo
Copy link
Contributor Author

@Ragazzo Ragazzo commented Jan 3, 2014

should this PR be adjusted according new changes of supporting input array options?

@qiangxue
Copy link
Member

@qiangxue qiangxue commented Jan 3, 2014

Yes, please. Thanks.

/**
* type of fixture generating
*/
const GENERATE_ALL = 'all_fixtures';

This comment has been minimized.

@qiangxue

qiangxue Jan 3, 2014
Member

I still don't quite like all_fixtures. Perhaps just use all?

This comment has been minimized.

@Ragazzo

Ragazzo Jan 3, 2014
Author Contributor

was thinking about the same all, sounds good, but is not it a little bit confusing? what all can user think?

This comment has been minimized.

@qiangxue

qiangxue Jan 3, 2014
Member

I think the command can be configured using fixture as the key, which will override the default fixture command.
Then all makes sense: yii fixture/generate all

This comment has been minimized.

@Ragazzo

Ragazzo Jan 3, 2014
Author Contributor

ok, will do changes. change docs also about fixture alias and not faker for the command?

This comment has been minimized.

@qiangxue

qiangxue Jan 3, 2014
Member

Yes. There's no need to have both fixture and faker commands in the same app. This only causes confusion.

This comment has been minimized.

@qiangxue

qiangxue Jan 3, 2014
Member

No, we can do it. If you configure it via controllerMap, it will take precedence when resolving route.

Also, I suggest we rename FakerController to be FixtureController.

This comment has been minimized.

@Ragazzo

Ragazzo Jan 3, 2014
Author Contributor

but if we do so, user will no be able to load his fixtures to db? what are you thinking about fixture:db and fixture:faker aliases? But since faker controller is extended from fixtures he also can load to db, true.

This comment has been minimized.

@Ragazzo

Ragazzo Jan 3, 2014
Author Contributor

Also, I suggest we rename FakerController to be FixtureController.

its already so, or what do you mean?

This comment has been minimized.

@qiangxue

qiangxue Jan 3, 2014
Member

... since faker controller is extended from fixtures he also can load to db,

Yes, faker controller has all the features of fixture controller. I don't see problem replacing the default fixture controller with faker.

This comment has been minimized.

@qiangxue

qiangxue Jan 3, 2014
Member

its already so, or what do you mean?

Forget about it. I didn't read carefully.

@Ragazzo
Copy link
Contributor Author

@Ragazzo Ragazzo commented Jan 3, 2014

Done, ready for review.

qiangxue added a commit that referenced this pull request Jan 3, 2014
Yii2 faker integration
@qiangxue qiangxue merged commit 389ef89 into yiisoft:master Jan 3, 2014
1 check was pending
1 check was pending
default The Travis CI build is in progress
Details
@qiangxue
Copy link
Member

@qiangxue qiangxue commented Jan 3, 2014

Thank you very much!

@Ragazzo Ragazzo deleted the Ragazzo:yii2_faker_integration branch Jan 3, 2014
@Ragazzo
Copy link
Contributor Author

@Ragazzo Ragazzo commented Jan 3, 2014

Good, in the extensions docs there was written that i also should ask you for sub-split and add some changes to the PhpDocController, will you do this?

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

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.