Configuration for custom namespaces #378

natepage · 2019-04-07T12:48:36Z

Hi there,

I'm working on projects which have the convention to put all the entities and repositories under App\Database\Entity and App\Database\Repository.

I've had a quick look at the issues and the PRs and I've seen other people asking for the same thing so I've decided to give it a go :)

Here is a simple solution to implement a helper to hold all the different namespaces, give it to the generator so it can be used in all makers.

This solution changes the constructor signatures of:

Symfony\Bundle\MakerBundle\Generator
Symfony\Bundle\MakerBundle\Doctrine\DoctrineHelper

Those objects are used with DI so it doesn't impact the existing makers but if anyone has extended them in their project it would then be a BC.

I leave this PR here and I'm looking forward to hearing from you.

weaverryan · 2019-04-09T00:13:16Z

Hey Nate!

I’ll do my best to review soon - I have a few other things to take care of in the short-term. But my first impression is that... this looks pretty sweet! It doesn’t cause any extra headache for users with the default setup and gives nice flexibility. Great idea!

Cheers!

natepage · 2019-04-10T10:06:49Z

Thank you for the answer, I'm glad of this positive feedback :)

Please let me know if there is anything I can do.

weaverryan

Some minor comments, but excellent work!!!

weaverryan · 2019-04-19T17:36:37Z

src/Util/NamespacesHelper.php

+    /**
+     * Get fixtures namespace.
+     *
+     * @return string


All the @return are unnecessary thanks to the return statement. And the description above doesn't add anything - so let's remove the phpdoc entirely.

weaverryan · 2019-04-19T17:37:42Z

src/Util/NamespacesHelper.php

+        $this->twig = $twig ?? 'Twig\\';
+        $this->unitTest = $unitTest ?? 'Tests\\';
+        $this->validator = $validator ?? 'Validator\\';
+    }


It's going to be weird either way, but I think we should just accept a $namespaces array, then do a check to make sure all the "keys" we expect are there (and throw an exception if any are missing or extra). I don't want to keep addming more and more arguments as we generate more and more stuff. But having the individual getters is ok.

@weaverryan Do we really need to check the given namespaces?

If a namespace isn't given we will default it and doing it in the helper directly has the advantage of having a single place to define the default value of a namespace.

If an extra namespace is given it won't have any impact because as you said we have the individual getters which look for specific namespaces so there will be no way to get an extra namespace from the namespaces helper.

But I completely agree with your point about keep adding more arguments each time we want to introduce a new namespace, so I've implemented a mix of our 2 solutions, please have a look and let me know what you think.

weaverryan · 2019-04-19T17:40:41Z

src/Generator.php


-    public function __construct(FileManager $fileManager, string $namespacePrefix)
+    public function __construct(FileManager $fileManager, NamespacesHelper $namespacesHelper)


For backwards compatibility, we need to temporarily allow for both. You would:

A) Remove the type-hint
B) If it's a string, use trigger_error to trigger a deprecation. The, manually create a NamespacesHelper with all the default values.

@weaverryan I've implemented the solution you've suggested and I'm also checking the type of the provided $namespacesHelper to make sure either a string or a NamespacesHelper instance is given because without the type hint it is the only way we have to enforce it.

Also please have a look at the exception and trigger_error messages because I wasn't sure about what to write :)

jschaedl · 2019-04-21T13:21:07Z

Will probably fix #363 and #368

natepage · 2019-04-23T00:50:18Z

@weaverryan Sorry I've just seen your comments it was a long weekend here in Australia so I've taken some time off the computer :) I will update the code according to your comments shortly. Thanks for the feedback.

natepage · 2019-05-02T12:37:31Z

@weaverryan Did you have a chance to have a look at the changes? And also the builds are failing because of the Translation component not being installed, did I do anything wrong? Thank you

weaverryan

Sorry for the delay - Symfony 4.3 kept me busy. This is very close now - a few minor comments. Also, we need a test for NamespacesHelper - we just need a very simple test for each method, which will guarantee we don't make some silly mistake in the future and break one of these methods without realizing it.

weaverryan · 2019-06-01T11:45:30Z

src/DependencyInjection/Configuration.php

+                ->scalarNode('subscriber_namespace')->defaultValue('EventSubscriber\\')->end()
+                ->scalarNode('twig_namespace')->defaultValue('Twig\\')->end()
+                ->scalarNode('unit_test_namespace')->defaultValue('Tests\\')->end()
+                ->scalarNode('validator_namespace')->defaultValue('Validator\\')->end()


I think we should move everything under a new namespaces key. That'll help in MakerExtension - instead of passing ALL the config to namespaces helper - including some extra keys that we may add in the future - it will only pass the namespaces. We'll need to still allow root_namespace at the root level however, but deprecate doing that. To make that happen, the new namespaces.root should default to null as well as the old, deprecated option I think. Then we can use a beforeNormalization() (see https://github.com/symfony/symfony/blob/4.4/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php) to check if the old key is set, but the new one is not. And if so, use the old key's value.

Let me know if that made no sense :p

weaverryan · 2019-06-01T12:03:35Z

src/Generator.php

+        }
+
+        if (\is_string($namespacesHelper)) {
+            @trigger_error('Passing a "string" as 2nd argument is deprecated since version 1.11.7.', E_USER_DEPRECATED);


weaverryan · 2019-06-01T12:05:15Z

src/Maker/MakeEntity.php

@@ -56,7 +57,7 @@ public function __construct(FileManager $fileManager, DoctrineHelper $doctrineHe

        if (null === $generator) {
            @trigger_error(sprintf('Passing a "%s" instance as 4th argument is mandatory since version 1.5.', Generator::class), E_USER_DEPRECATED);
-            $this->generator = new Generator($fileManager, 'App\\');
+            $this->generator = new Generator($fileManager, new NamespacesHelper());


Do we need to also pass root_namespace set to App\\?

If not given the root_namespace will default to App\\, we can probably explicitly set it to avoid confusion.

Any thoughts?

weaverryan · 2019-06-01T12:07:53Z

src/Util/NamespacesHelper.php

@@ -0,0 +1,107 @@
+<?php
+
+declare(strict_types=1);


remove this - it's just not something we use

weaverryan · 2019-06-01T12:08:37Z

src/Util/NamespacesHelper.php

+     * @param string $namespace
+     *
+     * @return string
+     */


no phpdoc needed - the @param and @return are redundant (thanks to type-hinting and return types 🎆) and the comment is self-evident.

This PR was squashed before being merged into the 1.0-dev branch (closes #379). Discussion ---------- More robust autoloader detection When the project, or dependent package, adds a custom autoloader, e.g. PhpStan, the added autoloader will be returned instead of the one containing the project's autoloader when `ComposerAutoloaderFinder` iterates the autoloader list leading to the mysterious error: ``` Could not determine where to locate the new class "App\Controller\DeliciousPuppyController", maybe try with a full namespace like "\My\Full\Namespace\DeliciousPuppyController" ``` Fixes #196 Fixes #231 Fixes #313 Related #378 Commits ------- 719979e Return objects from methods & switch fallback logic 22f5a3b Search for matching PSR-0 or PSR-4 namespaces, and default to component's autoloader 07830f9 Only return the autoloader when it contains the root namespace

xaoseric · 2019-12-05T14:02:45Z

Why not load the namespace from the composer file similar to how laravel does it in the getNamespace function?
https://github.com/laravel/framework/blob/6.x/src/Illuminate/Foundation/Application.php

natepage · 2020-01-07T11:34:19Z

@weaverryan Sorry for the delay I've been distracted on other projects 😄

I've tried to apply everything you've said in your comments.

Blackskyliner · 2022-08-23T08:12:47Z

What is needed to get this merged as this feature would make the MakerBundle much more usable in non-standard Installations (e.G. DDD coneptualized Applications).

c33s · 2022-08-23T12:05:30Z

@Blackskyliner maybe you also want to comment at #368

natepage · 2022-08-23T22:10:30Z

@Blackskyliner @c33s thank you for your interest in this PR 😄 TBH the work was done so long ago I'm actually not sure, I believe I've implemented the feedback I was given and was waiting for another review.
Now the code has changed quite a bit since, so I can see conflicts that would need to be addressed, but I think that's it

jrushlow · 2022-08-23T22:34:38Z

@natepage - if you can get this rebased to resolve the conflicts with passing tests, ill get another review on this pretty quick so we can merge. Thanks in advanced!

natepage · 2022-08-30T15:33:42Z

@jrushlow I've rebased the PR to the latest main, however I have this issue with setting up the database for tests

Exception: Could not find "***127.0.0.1:5432/app?serverVersion=13&charset=utf8" inside ".env"

Could you please guide me a little bit on how to resolve this?

jrushlow · 2022-09-13T13:43:55Z

Howdy @natepage - the issue was with our CI and i've just merged a fix for this in #1179. If you rebase, the testsuite should run correctly. There may be a few unrelated failures, but nothing to worry about. I'm whipping up a fix for those in #1180 now. Thanks for the work on this!

natepage · 2022-10-20T12:07:05Z

Hey @jrushlow, sorry for the late reply I got caught up with life.. 😄
I've synced my PR and most CI is green excepted some tests around CRUD generation with PHP8.1 and Syfmony 6.2, I've tried to have a quick look at it but I didn't have enough time to fully understand how the tests are running.
Do you think it's related to my changes?

cavasinf · 2023-09-12T12:26:34Z

I would love to have this feature for our Symfony bundle!
@natepage, can you rebase your PR to main and see if the errors are still there?

natepage · 2023-10-02T05:49:54Z

@cavasinf sorry for the late reply, I've synced my PR with main, fixed conflicts and addressed coding standards.

However, some tests are failing because it looks like the ExpressionLanguage isn't getting installed with the given dependencies config.

Not too sure what to do with this 😄

richardrodriguez21 · 2024-02-24T15:32:02Z

Hello everyone, what is missing? Is there anything I can help with?

ToshY · 2025-01-01T16:09:55Z

@jrushlow If this PR gets rebased with main, is there any chance it will get merged this year?
@natepage Are you willing to update the PR if so?

natepage · 2025-01-01T20:26:38Z

@ToshY of course! If this PR is still relevant to this day then I'm more than happy to contribute 😄

natepage · 2025-03-31T09:18:56Z

@ToshY sorry for the late action here, life got in the way.. 😄

This PR has been seating for quite a few years so I've actually went through it all over again, it was actually way easier than resolving conflicts and making sure I don't break anything.

But I kept it consistent with the original implementation, the only differences are:

using the new bundle class structure
add namespaces for recent components (e.g. Scheduler, Webhook, ...)

Anyway it's all there, and as far as I can tell the issues with tests are related to #1662

I'm not sure what to do for this PR to get some attention, but I would recommed to get it merged sonner rather than later because keeping it in sync is a bit tidious, and to be completely honest I'm not personally using this bundle anymore so it would be great if it becomes part of it so other people can keep evolving it!

GwendolenLynch mentioned this pull request Apr 16, 2019

More robust autoloader detection #379

Merged

weaverryan reviewed Apr 19, 2019

View reviewed changes

weaverryan reviewed Jun 1, 2019

View reviewed changes

weaverryan added the Status: Needs Work Additional work is needed label Jun 14, 2019

natepage requested a review from weaverryan January 12, 2020 01:54

weaverryan changed the base branch from master to main November 16, 2020 20:03

jrushlow added the Feature New Feature label May 10, 2022

c33s mentioned this pull request Aug 23, 2022

Custom namespace by makers #368

Closed

natepage force-pushed the feature/custom-namespaces branch from 3c82ba6 to 5abd1fb Compare August 30, 2022 14:52

Implement NamespacesHelper on fresh codebase

a4d76ad

natepage force-pushed the feature/custom-namespaces branch from 6c3f39c to a4d76ad Compare March 31, 2025 07:44

natepage added 5 commits March 31, 2025 18:54

Fix ClassData::create() calls

543a1cb

Fix InteractiveSecurityHelper constructor calls

04f1fdf

Fix namespace in MakeEntity

48c6e43

Fix ClassDataTest rootNamespace

ecd80f3

Fix MakeSchedule messageClass

eec39a0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Configuration for custom namespaces #378

Configuration for custom namespaces #378

natepage commented Apr 7, 2019

weaverryan commented Apr 9, 2019

natepage commented Apr 10, 2019

weaverryan left a comment

weaverryan Apr 19, 2019

weaverryan Apr 19, 2019

natepage Apr 23, 2019

weaverryan Apr 19, 2019

natepage Apr 23, 2019

jschaedl commented Apr 21, 2019 •

edited

Loading

natepage commented Apr 23, 2019

natepage commented May 2, 2019

weaverryan left a comment

weaverryan Jun 1, 2019

weaverryan Jun 1, 2019

weaverryan Jun 1, 2019

natepage Jan 7, 2020

weaverryan Jun 1, 2019

weaverryan Jun 1, 2019

xaoseric commented Dec 5, 2019 •

edited

Loading

natepage commented Jan 7, 2020

Blackskyliner commented Aug 23, 2022

c33s commented Aug 23, 2022

natepage commented Aug 23, 2022

jrushlow commented Aug 23, 2022

natepage commented Aug 30, 2022

jrushlow commented Sep 13, 2022

natepage commented Oct 20, 2022

cavasinf commented Sep 12, 2023

natepage commented Oct 2, 2023

richardrodriguez21 commented Feb 24, 2024

ToshY commented Jan 1, 2025

natepage commented Jan 1, 2025

natepage commented Mar 31, 2025


		public function __construct(FileManager $fileManager, string $namespacePrefix)
		public function __construct(FileManager $fileManager, NamespacesHelper $namespacesHelper)

		@@ -0,0 +1,107 @@
		<?php

		declare(strict_types=1);

Configuration for custom namespaces #378

Are you sure you want to change the base?

Configuration for custom namespaces #378

Conversation

natepage commented Apr 7, 2019

weaverryan commented Apr 9, 2019

natepage commented Apr 10, 2019

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

jschaedl commented Apr 21, 2019 • edited Loading

natepage commented Apr 23, 2019

natepage commented May 2, 2019

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

xaoseric commented Dec 5, 2019 • edited Loading

natepage commented Jan 7, 2020

Blackskyliner commented Aug 23, 2022

c33s commented Aug 23, 2022

natepage commented Aug 23, 2022

jrushlow commented Aug 23, 2022

natepage commented Aug 30, 2022

jrushlow commented Sep 13, 2022

natepage commented Oct 20, 2022

cavasinf commented Sep 12, 2023

natepage commented Oct 2, 2023

richardrodriguez21 commented Feb 24, 2024

ToshY commented Jan 1, 2025

natepage commented Jan 1, 2025

natepage commented Mar 31, 2025

jschaedl commented Apr 21, 2019 •

edited

Loading

xaoseric commented Dec 5, 2019 •

edited

Loading