Add configuration of root namespace #173

upyx · 2018-05-04T16:18:29Z

This PR adds configuration for namespaces used in make:* commands that was discussed in #133.
I've used this doc in work and examples from FrameworkBundle.

New parameter:
maker.root_namespace - application root namespace, default: App

To change it, you should create configuration file in config/packages/dev with name maker.yaml or maker.xml or maker.php with root node maker. For example YAML file:

# config/packages/dev/maker.yaml
maker:
    root_namespace: 'App'

I've added some tests. I think we don't need Flex recipe but just document new feature.
I need help for write documentation because my English.

TODO:

Tests
Documentations
Squash commits

LeJeanbono · 2018-05-04T17:06:25Z

Doc and Recipe sound good !
I'll take a look
Nice job ;)

LeJeanbono · 2018-05-04T17:17:37Z

You did a conf for entity namespace, should we have for others classes (Form, Repo, Twig, etc)?

upyx · 2018-05-04T17:57:26Z

@LeJeanbono Yes, I think we should. This PR is WIP.

weaverryan · 2018-05-06T02:36:32Z

Hey @upyx!

Woo! Thanks for starting this! I have some thoughts:

I think we should only make the root_namespace configurable, at least for now. This bundle is highly opinionated on purpose, and isn't intended to work for all edge cases. Besides, if you really do want to, for example, generate a form in a different directory, you can do that by passing the \Full\Namespace. So, only include root_namespace. It will be much faster to finish & merge this PR anyways.
I don't think we should update the recipe to create the config file, because 99% of people won't need it. Let's just document it. Actually, (maybe in a future PR), it would be cool to detect when the user is having problems using the App\ namespace (e.g. they try to generate some entity called Foo, and we determine that there is no App\ in their autoloader), ask them if they want to define a different root namespace, then create the config file for them. After all, MakerBundle is all about generating code :)

Cheers!

upyx · 2018-05-06T08:10:55Z

Hi @weaverryan !
At the first look I thought to change only root namespace. But in our project we have many legacy code and have non standard directories. So configuration of many namespaces will be very helpful for our project and may be other. But I understand that our project only one of all. If you are absolutely sure this feature isn't practical then I remove configuration of entity namespace. But think again. We can do bundle more configurable without any disadvantages. Why not do it?

weaverryan · 2018-05-06T02:37:58Z

src/DependencyInjection/MakerExtension.php

+    public function getAlias()
+    {
+        return 'maker';
+    }


I don't believe this is needed

Oh, I'm not sure... I will be grateful if you explain it. I see doctrine and many others do the same. As I understand, result of this method won't cached. So avoiding it (as you say earlier) has minor negative performance impact.

weaverryan · 2018-05-06T02:38:06Z

src/DependencyInjection/MakerExtension.php

+    public function getConfiguration(array $config, ContainerBuilder $container)
+    {
+        return new Configuration();
+    }


This also should not be needed

weaverryan · 2018-05-06T02:42:05Z

src/DependencyInjection/MakerExtension.php

+        $configuration = $this->getConfiguration($configs, $container);
+        $config = $this->processConfiguration($configuration, $configs);
+
+        $container->setParameter('maker.root_namespace', $config['root_namespace']);


Hmm, we should not create a parameter if we can avoid it - it has minor negative performance impact, and there's just not advantage.

But, making this happen IS a bit technical, so I'll explain, but let me know if you need help. The process is:

First, we need to create an "abstract" service in services.xml that basically registers this service:

maker-bundle/src/DependencyInjection/CompilerPass/MakeCommandRegistrationPass.php

Lines 35 to 41 in 5110979

$container->register(

sprintf('maker.auto_command.%s', Str::asTwigVariable($class::getCommandName())),

MakerCommand::class

)->setArguments([

new Reference($id),

new Reference('maker.file_manager'),

])->addTag('console.command', ['command' => $class::getCommandName()]);

(this is basically a "template" service).

2 In the compiler pass, we would then use ChildDefinition to effectively "duplicate" that service and make any changes we need. Here's an example from core: https://github.com/symfony/symfony/blob/cada38f520784cc3f248874ad268e76fc99ba8d9/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php#L563

In this class, instead of creating a parameter, we would simply set the 3rd argument to the abstract service (created in step 1) to this value.

Use abstract service is great idea! I'll try it.

However where I can get root_namespace in DI context (compiler pass or service.xml) if it is not stored as "parameter"?

The second, if we are deciding to configure root namespace only, it can be more practical - to remove config and use just a parameter. What do think?

P.S.
My English leaves much to be desired. Please, fill free to correct me and ask to explain.

Ah, great question. And, you've done a REALLY good job so far making this all work with ChildDefinition :).

For this last part, in this file, you should fetch the maker.auto_command.abstract definition and HERE set its first argument to $config['root_namespace']. You're doing this in the compiler pass right now. Just move that one part of the code here, and then you won't need to rely on the parameter.

Oh, and for DoctrineHelper, instead of referencing the %maker.root_namespace% parameter in services.xml, you will also just leave that blank in services.xml, and set the argument manually in this class.

JSkalskiSBG · 2018-05-10T10:03:33Z

Other ways to fix it for 'advanced users with other namespace then App' would be:

check namespace of Kernel.php
handle namespace parameter:

bin/console maker:command my-command --namespace Custom\\Namespace\\Project

handle namespace given in name:

bin/console maker:command \\Custom\\Namespace\\Project\\my-command
-- OR:
bin/console maker:command \\Custom\\Namespace\\Project\\Command\\my-command

Right now 3rd option gives error:

[ERROR] The "\Custom\Namespace\Project\Command\my-command" command name is not valid because it would be implemented by
"\Custom\Namespace\Project\Command\myCommand" class, which is not valid as a PHP class name (it must start with
a letter or underscore, followed by any number of letters, numbers, or underscores).

weaverryan · 2018-05-10T10:49:13Z

About that error - that looks like a bug to me - we may have too much validation on make:command. We need to fix that :)

upyx · 2018-05-10T11:47:28Z

Guys! Let's finish this work.

Could you help me with these questions:

Does implementation of getAlias() and getConfiguration() methods interfere to performance? ~~Should I really delete these method?~~ I have deleted these method.
What do we configure the root namespace only or namespaces for all commands?
If we configure root namespace only can I remove configuration file and use parameters?
If we continue to use the configuration file how parameters can be removed?
Will we use a recipe?
What tests should I add?

@weaverryan what do you think about it?

JSkalskiSBG · 2018-05-10T14:54:48Z

About this solution:
It looks bad with current config file:

root_namespace: 'App'
entity_namespace: '%maker.root_namespace%\Entity'

If we got special config for 'entity', why not special config for 'command'?
Someone may want to keep it in MySpace\Console, so adding 'Command' to namespace is bad idea.
Someone may also use other directory than 'src/Command'.

I think that options '--namespace My\Name\Space\CustomDir' and '--dirpath src/CustomDir' would be much better (directory path relational to folder with composer.json).

Issue should be renamed to 'Add configurable namespace and file path'.
Code that fix this issue should fix it in all 'make' commands, not only 'make:entity' and 'make:command'.

About performance:
maker-bundle should be installed only in 'dev' environment.
Does anyone know, if loading config will affect perfomance after installation on production without 'require-dev' from composer?

upyx · 2018-05-11T06:55:11Z

Hello @LeJeanbono! I've already removed entity_namespace from configuration as @weaverryan advised. But I prefer to add more configurable options. However we still able to add it later. You are right about dev environment so we shouldn't care about <1ms performance impact. I don't like idea about options because it requires to type more letters every time. But really benefit of this bundle it is creation files quick and simple.

Symfony uses aggressive caching of configuration so performance very weak depends on configuration size. However default App\Kernel class has that string:

$loader->load($confDir . '/{packages}/*' . self::CONFIG_EXTS, 'glob');
$loader->load($confDir . '/{packages}/' . $this->environment . '/**/*' . self::CONFIG_EXTS, 'glob');

So if you will place maker.yaml to config/packages directory it would be affected on production environment. But you can place one in config/packages/dev to fix it.

upyx · 2018-05-11T10:04:47Z

I've added tests for changed namespace cases. @weaverryan could you check it, please?

LeJeanbono · 2018-05-13T14:42:14Z

Done for dev env @upyx

weaverryan

Hey @upyx!

Nice work - we're close, I've left some more comments :).

I also have 3 other requests, if you want to try them!

For tests, make sure we have a MakeEntity test, and one that has a relation. I want to make sure that you can (A) generate an entity correctly when your root namespace is changed and (B) the system correctly looks in the custom namespace for the relation (e.g. if I just type that I want to relate to a Comment entity, it will look for that in Custom\Entity\Comment. I also want to make sure that the repository class generates into the Custom\Repository namespace, there is a current bug where it generates into App\Repository,
At the beginning of ALL commands (so, probably in MakerCommand, if we do not detect that the user's app has any autoload rules for their root_namespace, let's add a warning:

It looks like your app may be using a namespace other than "ROOT_NAMESPACE_HERE". see https://link-to-docs for how to configure this.

And finally, we DO need to add a little note in the docs for this bundle about this new option :). I don't think we should have a recipe.

weaverryan · 2018-05-13T16:43:31Z

src/DependencyInjection/CompilerPass/MakeCommandRegistrationPass.php

-                new Reference('maker.file_manager'),
-            ])->addTag('console.command', ['command' => $class::getCommandName()]);
+            $commandDefinition = new ChildDefinition('maker.auto_command.abstract');
+            $commandDefinition->setClass(MakerCommand::class);


I think this is not needed, because you already specified it in services.xml

It is needed - actually class not inherits to child services. However it may be bug. Documentation says:

All attributes on the parent service are shared with the child except for shared, abstract and tags. These are not inherited from the parent.

I debugged it. Really class inherits. It is resolved in ResolveChildDefinitionsPass but AddConsoleCommandPass calls earlier (when class is set to null).

weaverryan · 2018-05-13T16:46:39Z

src/DependencyInjection/MakerExtension.php

+        $configuration = $this->getConfiguration($configs, $container);
+        $config = $this->processConfiguration($configuration, $configs);
+
+        $container->setParameter('maker.root_namespace', $config['root_namespace']);


Ah, great question. And, you've done a REALLY good job so far making this all work with ChildDefinition :).

For this last part, in this file, you should fetch the maker.auto_command.abstract definition and HERE set its first argument to $config['root_namespace']. You're doing this in the compiler pass right now. Just move that one part of the code here, and then you won't need to rely on the parameter.

weaverryan · 2018-05-13T16:52:52Z

src/DependencyInjection/MakerExtension.php

+        $configuration = $this->getConfiguration($configs, $container);
+        $config = $this->processConfiguration($configuration, $configs);
+
+        $container->setParameter('maker.root_namespace', $config['root_namespace']);


Oh, and for DoctrineHelper, instead of referencing the %maker.root_namespace% parameter in services.xml, you will also just leave that blank in services.xml, and set the argument manually in this class.

weaverryan · 2018-05-13T17:01:05Z

src/Maker/MakeEntity.php


-    public function __construct(FileManager $fileManager, string $projectDirectory, DoctrineHelper $doctrineHelper)
+    public function __construct(FileManager $fileManager, DoctrineHelper $doctrineHelper, string $projectDirectory, string $entityNamespace)


To avoid needing to inject the $entityNamespace ALSO here, let's add a getEntityNamespace() method to DoctrineHelper.

weaverryan · 2018-05-13T17:06:33Z

src/Test/MakerTestDetails.php

+                'phpunit.xml.dist',
+                '<env name="KERNEL_CLASS" value="App\\Kernel" />',
+                '<env name="KERNEL_CLASS" value="'.$rootNamespace.'\\Kernel" />'
+            );


I don't know if we need to do all of this. I should be enough to create the maker.yaml file with the config AND add one new PSR4 namespace autoload to composer.json. I don't think we need to completely eliminate and convert the rest of the app from the App\ namespace.

At first I did so. But such solution has issue: tests pass when it mustn't because autoloader. After debugging tests we can remove it. But I think complete converting namespace is better because it is case for we are doing all of this 😄

weaverryan · 2018-05-13T17:09:01Z

tests/Maker/FunctionalTest.php

+            ->changeRootNamespace('Custom')
+            ->addPreMakeCommand('composer dump-autoload')
+        ];
+


I think we only really need to test one case, because each maker is using the exact same code to place files. Well, maybe 2 cases: MakeCommand (as a general case) and MakeEntity, because it is indeed a bit more complex.

It is. I will remove useless tests

upyx · 2018-05-14T11:01:37Z

Thank you for review @weaverryan! I've done changes by your requests. Could you review again, please?
And may I ask you to do PR in documentation?

…space

weaverryan

This is great! I think this is the last round of changes :)

weaverryan · 2018-05-17T15:29:26Z

src/Command/MakerCommand.php

+                    $this->rootNamespace
+                )
+            );
+        }


Instead of an exception, let's convert this to a warning. Something like:

$this->io->note([ sprintf('It looks like your app may be using a namespace other than "%s".', $this->rootNamespace), 'See https://symfony.com/doc/current/bundles/SymfonyMakerBundle/index.html#configuration to configure this. ]);

The #configuration part of the link doesn't exist yet, but we'll add it.

weaverryan · 2018-05-17T15:30:26Z

src/FileManager.php

@@ -163,6 +163,11 @@ public function getNamespacePrefixForClass(string $className): string
        return $this->autoloaderUtil->getNamespacePrefixForClass($className);
    }

+    public function isNamespaceConfiguredToAutoload(string $namespace): string


: bool return type

weaverryan · 2018-05-17T15:30:56Z

src/Util/AutoloaderUtil.php

+     *
+     * @return bool
+     */
+    public function isNamespaceConfiguredToAutoload(string $namespace)


Add : bool return type, and remove the @param and @return - they are duplicated by the code itself.

weaverryan · 2018-05-17T15:33:41Z

tests/Command/MakerCommandTest.php

+        $command->setName('make:foo');
+        $tester = new CommandTester($command);
+        $tester->execute(array());


I like this test. We'll just need to instead look at the output, instead of the exception when we convert it to just a message.

weaverryan · 2018-05-17T15:53:02Z

Actually, I just made all the changes - including the docs - because they were so minor. Let's see if the tests still pass!

* master: improve make:form arguments description Make the Generator class a service Add a functional test Check if the class exists Add an interactive question API Platform support: add an --api-resource flag for the make:entity command

weaverryan · 2018-05-17T16:10:19Z

Also updated after a nasty merge conflict with a related PR. Let's see if the tests pass. @upyx can you check out the latest code to make sure the conflict didn't mess anything up? A lot of the same lines of code were changed.

upyx · 2018-05-17T17:36:45Z

Oh! Great! I see you strongly determined to comlete this 😃

weaverryan · 2018-05-17T17:38:09Z

Ha - I am! It’s a missing feature. I knew that :). And I’ll make a release as soon as it’s done.

weaverryan · 2018-05-17T19:21:42Z

Thanks for you work @upyx! The test failures are unrelated, they need to be fixed on master.

…averryan) This PR was merged into the 1.0-dev branch. Discussion ---------- Add configuration of root namespace This PR adds configuration for namespaces used in `make:*` commands that was discussed in #133. I've used [this doc](https://symfony.com/doc/current/bundles/configuration.html) in work and examples from `FrameworkBundle`. New parameter: `maker.root_namespace` - application root namespace, default: `App` To change it, you should create configuration file in `config/packages/dev` with name `maker.yaml` or `maker.xml` or `maker.php` with root node `maker`. For example YAML file: ```yaml # config/packages/dev/maker.yaml maker: root_namespace: 'App' ``` I've added some tests. I think we don't need Flex recipe but just document new feature. I need help for write documentation because my English. TODO: - [x] Tests - [x] Documentations - [ ] Squash commits Commits ------- 42b7008 Fixing accidental removal of property 545a422 Merge branch 'master' into configurable_namespace b92f19c Merge branch 'master' into configurable_namespace bf1b092 updating for now-missing property b5658c2 Merge branch 'master' into configurable_namespace 7440ade return type stuff 656116c Changing from an exception 5605d6e adding docs 23a715c Merge remote-tracking branch 'upstream/master' into configurable_namespace 152ffa6 The most important fix f34fa39 Add exception with hint message when root namespace is not found 758a2c9 Refactor AutoloaderUtil 5423250 Remove maker.root_namespace parameter and improve tests 7157e88 Fix file creation on Windows 657f653 Remove useless code a425953 Add functional tests for custom root namespace cases d4139d4 Make "abstract" command and remove entity namespace from config 51d7138 Add configuration of root and entity namespaces

upyx · 2018-05-18T05:35:25Z

I am glad to work with you :)

Add configuration of root and entity namespaces

51d7138

upyx force-pushed the configurable_namespace branch from 86ad830 to 51d7138 Compare May 4, 2018 16:34

LeJeanbono mentioned this pull request May 4, 2018

[WIP] Config for maker-bundle symfony/recipes#404

Closed

weaverryan requested changes May 6, 2018

View reviewed changes

Make "abstract" command and remove entity namespace from config

d4139d4

upyx force-pushed the configurable_namespace branch from ff6c4f2 to d4139d4 Compare May 7, 2018 06:14

Sergey Rabochiy added 2 commits May 11, 2018 16:31

Add functional tests for custom root namespace cases

a425953

Remove useless code

657f653

upyx changed the title ~~[WIP] Add configuration of root and entity namespaces~~ [WIP] Add configuration of root namespaces May 11, 2018

upyx changed the title ~~[WIP] Add configuration of root namespaces~~ [WIP] Add configuration of root namespace May 11, 2018

Fix file creation on Windows

7157e88

weaverryan requested changes May 13, 2018

View reviewed changes

Sergey Rabochiy added 4 commits May 14, 2018 13:40

Remove maker.root_namespace parameter and improve tests

5423250

Refactor AutoloaderUtil

758a2c9

Add exception with hint message when root namespace is not found

f34fa39

The most important fix

152ffa6

Merge remote-tracking branch 'upstream/master' into configurable_name…

23a715c

…space

weaverryan requested changes May 17, 2018

View reviewed changes

weaverryan added 3 commits May 17, 2018 11:39

adding docs

5605d6e

Changing from an exception

656116c

return type stuff

7440ade

weaverryan added 2 commits May 17, 2018 12:05

updating for now-missing property

bf1b092

weaverryan changed the title ~~[WIP] Add configuration of root namespace~~ Add configuration of root namespace May 17, 2018

weaverryan and others added 3 commits May 17, 2018 11:14

Merge branch 'master' into configurable_namespace

b92f19c

Merge branch 'master' into configurable_namespace

545a422

Fixing accidental removal of property

42b7008

weaverryan merged commit 42b7008 into symfony:master May 17, 2018

javiereguiluz mentioned this pull request Jul 6, 2018

'Make:entity' failed when don't use the default namespace 'App'. #133

Closed

upyx deleted the configurable_namespace branch July 20, 2021 17:08

	$container->register(
	sprintf('maker.auto_command.%s', Str::asTwigVariable($class::getCommandName())),
	MakerCommand::class
	)->setArguments([
	new Reference($id),
	new Reference('maker.file_manager'),
	])->addTag('console.command', ['command' => $class::getCommandName()]);


		public function __construct(FileManager $fileManager, string $projectDirectory, DoctrineHelper $doctrineHelper)
		public function __construct(FileManager $fileManager, DoctrineHelper $doctrineHelper, string $projectDirectory, string $entityNamespace)

Add configuration of root namespace #173

Add configuration of root namespace #173

Conversation

upyx commented May 4, 2018 • edited by weaverryan Loading

LeJeanbono commented May 4, 2018 • edited Loading

LeJeanbono commented May 4, 2018

upyx commented May 4, 2018

weaverryan commented May 6, 2018

upyx commented May 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JSkalskiSBG commented May 10, 2018

weaverryan commented May 10, 2018

upyx commented May 10, 2018 • edited Loading

JSkalskiSBG commented May 10, 2018

upyx commented May 11, 2018

upyx commented May 11, 2018

LeJeanbono commented May 13, 2018

weaverryan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

upyx commented May 14, 2018 • edited Loading

weaverryan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weaverryan commented May 17, 2018

weaverryan commented May 17, 2018

upyx commented May 17, 2018

weaverryan commented May 17, 2018

weaverryan commented May 17, 2018

upyx commented May 18, 2018

upyx commented May 4, 2018 •

edited by weaverryan

Loading

LeJeanbono commented May 4, 2018 •

edited

Loading

upyx commented May 10, 2018 •

edited

Loading

upyx commented May 14, 2018 •

edited

Loading