Skip to content

Registry factory WIP#13

Closed
pepakriz wants to merge 2 commits intophpstan:masterfrom
pepakriz:registry-factory
Closed

Registry factory WIP#13
pepakriz wants to merge 2 commits intophpstan:masterfrom
pepakriz:registry-factory

Conversation

@pepakriz
Copy link
Contributor

Věci k vyřešení:

  • jaký název tagu?
  • neudělat z $tagToService samostatnou static helper třídu? NetteContainerHelpers? Případně udělat vlastní bridge na nette container (v budoucnu jen switchneš na symfony)?
  • opravdu zrušit public metodu register v registru? Za mě ano, ale je to bc break
  • dříve nebyl zaregistrován DefinedVariableInAnonymousFunctionUseRule. Nevím, jestli to byla chyba nebo záměr

@hrach
Copy link
Contributor

hrach commented Sep 27, 2016

why not findByType( \PHPStan\Rules\Rule::class) ?

@JanTvrdik
Copy link
Contributor

@hrach Because that's evil.

@hrach
Copy link
Contributor

hrach commented Sep 27, 2016

@JanTvrdik evil is the response which does not state any real arguments.

@ondrejmirtes
Copy link
Member

"Evil" does not mean anything, but I also dislike findByType in this case.
I'd rather have explicit configuration what is and isn't a rule to
register. It also allows to unregister rules by overriding the service
configuration in the future (although configuring the tool with Neon DI is
a temporary measure until PHPStan becomes more stable to desgin "final"
configuration).

On Tuesday, 27 September 2016, Jan Škrášek notifications@github.com wrote:

@JanTvrdik https://github.com/JanTvrdik evil is the response which does
not state any real arguments.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#13 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAGZuMYZRBlO5ZB54A5O-LfJA3qRZN9Bks5quNCzgaJpZM4KHWe9
.

Ondřej Mirtes

@TomasVotruba
Copy link

What is added value of tag in DI over implemented interface?

/** @var \Nette\DI\Container */
private $container;

public function __construct(\Nette\DI\Container $container)
Copy link

@TomasVotruba TomasVotruba Sep 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This duplicates tag and DI logic and violates few SOLID principles. No need for that.

Rather use Extension and beforeCompile() method:

class PhpStanExtension extends CompilerExtension
{

    public function beforeCompile()
    {
        $containerBuilder = $this->getContainerBuilder();

        $registryDefinition = $containerBuilder->getDefinition($containerBuilder->getByType(Registry::class);

        // or findByTag(), whatever you prefer        
        foreach ($containerBuilder->findByType(Rule::class) as $definitionName => $definition) {
        $registryDefinition->addSetup('register', ['@' . $definitionName]);
        }
    }
}

}
}

private function register(Rule $rule)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More ways to adjust class behavior usually leads to more bugs.
I'd recommend using one and adding another WHEN really needed.

@hrach
Copy link
Contributor

hrach commented Sep 27, 2016

@ondrejmirtes yep, this is an argument! Though, doing this with factory, which dependes on the DI, imo not a nice solution.

@ondrejmirtes
Copy link
Member

Please stop this bikeshedding, it's really not important 😊

I don't want a compiler extension because I want to get rid of the Nette
dependency later and yes, I like more verbose and flexible configuration.

On Tuesday, 27 September 2016, Jan Škrášek notifications@github.com wrote:

@ondrejmirtes https://github.com/ondrejmirtes yep, this is an argument!
Though, doing this with factory, which dependes on the DI, imo not a nice
solution.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#13 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAGZuByvbMUXcI81ovHgxNj6XJ-tCVZFks5quNVbgaJpZM4KHWe9
.

Ondřej Mirtes

@TomasVotruba
Copy link

TomasVotruba commented Sep 27, 2016

What is bikeshedding?

You don't want to use Nette\DI or and DI component?

In second case, you are starting creating own DI.
This might be flexible at start, yet breaking SOLID and empowering others to do so.

Just wonder, what are your reasons (apart feelings) to not to use Nette or any DI?

@ondrejmirtes
Copy link
Member

Obsessing over some minor detail where the value of the whole thing is completely unrelated :) See https://en.wikipedia.org/wiki/Law_of_triviality.

I expect the neon service config completely gone by 0.5 or 0.6, so I don't want to discuss something that is not there for the long term and therefore doesn't matter.

For anyone interested, this is the current roadmap for 0.2: https://trello.com/b/r9mZ7SZ0/phpstan But please, if you want to implement a feature, first discuss it with me before sending a PR, I have some ideas about how the codebase should evolve.

@TomasVotruba
Copy link

I see. We didn't know you don't want to use Nette DI nor Neon. This explains a lot from your messages :)

I didn't find answer for my last question.

Just wonder, what are your reasons (apart feelings) to not to use Nette or any DI?

"Don't want to explain" is also an answer.

@ondrejmirtes
Copy link
Member

I think that having a separate DI container is not a very good idea, no similar tool I know does that. I included it for current versions because it helped to speed up the development until I have a better idea how to approach this problem of creating my own objects.

One way to do this is to have a naked phpstan-core and phpstan-nette/phpstan-symfony bridges with its own container extensions. That would also allow to share services between the analyzed app and PHPStan class reflection extensions which is sometimes needed and currently solvable only by copy-pasting configuration.

@TomasVotruba
Copy link

That convention might be as same as were static to DI. Nobody used DI, because everybody used static. Tradition is not always good reason to do something.

I use DI for similar tools and I don't see any disadvantages. That's why I asked.

I have a better idea how to approach this problem of creating my own objects.

I look forward to this, if you are really determined and will come with something better than DI container.

@ondrejmirtes
Copy link
Member

Merged as 45eed0f (build was failing due to a dep conflict which I solved earlier today in masterú. Thanks!

@evandrojr evandrojr mentioned this pull request Jul 10, 2023
@wehostadm wehostadm mentioned this pull request Nov 17, 2023
@johntheteam johntheteam mentioned this pull request Aug 17, 2024
@klewi1983 klewi1983 mentioned this pull request May 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants