Refactor Generators

[mostafakhudair]

Refactor generators commands to match with standard commands with additional useful options

All generators extends BaseCommand using GeneratorTrait
Fix Controller extends PresenterController methods
Auto generate Entity class on make:model with option -return entity
for example:
php spark make:model users -table users -return entity
will result
App\Models\Users
App\Entities\Users

Add suffix class name by prepend component name, with --suffix option
for example:
php spark make:controller users
will result
App\Controllers\Users

php spark make:controller users
will result
App\Controllers\UsersController

Scaffold command removed for now and will be added in a separate PR later

Simplify code and clean unnecessary vars

userguide will be updated in a separated PR

• Securely signed commits
• Component(s) with PHPdocs
• Unit testing, with >80% coverage
• User guide updated
• Conforms to style guide

[MGatner]

This looks like a good move, but I will need to review it more thoroughly on desktop. Also looks like some static analysis failures, so maybe WIP? We would need to have this done before release in order to avoid breaking changes on the generator class names.

[mostafakhudair]

Thanks @MGatner , and I'm working on it, I hope if i finish it today

[paulbalandan]

Generally, I like the refactors made. I'll do a full review tomorrow but my initial concern is on programmatic calls. CLI::getOption() will return nothing when the command is not called via cli, that is why we need to check array_key_exists on the $params. This way, the options can be tested via unit tests. So, please do not remove those array_key_exists checks already in place.

Another one is on the --force option handling. This option is meant to allow overwriting files but you made it impossible to overwrite even if --force is given. It is like you are offering something and then when they grab that you refuse to give that right away. I find it quite illogical.

[mostafakhudair]

file overwritten fine with --force option
I just put force check just after file is replaced

[paulbalandan]

Alright, then. I maybe got quite confused with that block.

Please then re-include the access to $params for the options so it will be possible to use command('make:command foo --bare) and others programmatically. Also, please check the changes you've made to the unit tests and inquire why it runs "forever" and stuck at 4%. Maybe you inadvertently made a recursion error or something.

[paulbalandan]

These review comments are not yet exhaustive but my initial comments only. Most of the comments are on the proper programmatic call handling.

[paulbalandan]

I guess the 360 minute testing was caused by an executed call to CLI::prompt which keeps PHPUnit waiting for a keyboard input.

[mostafakhudair]

@paulbalandan Thanks for this helpful review, give me few hours to be in home and i'll fix all of this mistakes

[MGatner]

Nice work team! I came to see where this was at, I like seeing all the activity.

[mostafakhudair]

CLI::promot is ignored now but phpunit still stuck, any help on this? @paulbalandan

[sfadschm]

@mostafakhudair I ran the test locally one by one and indicated where looping happens. Seems to be related to the make:model command. Hope this helps.

[mostafakhudair]

@sfadschm Thanks for your time to test this, but could you tell me why specially make:model looping

[sfadschm]

Btw. what happened to the model table defaulting to pluralized class name?

CodeIgniter4/system/Commands/Generators/CreateModel.php

Lines 127 to 142 in 453af97

	$table = $this->params['table'] ?? CLI::getOption('table');
	if (! is_string($table))
	{
	$table = str_replace($this->getNamespace($class) . '\\', '', $class);
	}
	if ($pos = strripos($table, 'Model'))
	{
	$table = substr($table, 0, $pos);
	}
	// transform class name to lowercased plural for table name
	$table = strtolower(plural($table));
	return str_replace('{table}', $table, $template);

[mostafakhudair]

Test ran well on local machine, but still stucked at 6% in here

[sfadschm]

Thats still MigrationGeneratorTest. See comment somewhere above (kind of hard to follow the changes with all the force pushes :D).
So check the MigrationGenerator for required prompts and you should be fine.

[mostafakhudair]

I already fixed model and migrate prompt before i push changes

[paulbalandan]

I saw a lot of comments here. Some are on the deviation from the original logic of GeneratorCommand, others on the design, while others is removing existing functionalities. I saw some changes removing existing functions even before GeneratorCommand was existing, so technically that removal is a BC break for existing users. I'll have to put the BC break tag until those BCs are resolved.

Also, don't dismiss right away a comment without providing an explanation. If you do not agree, tell us so.