Adding support for extensions and PSR-11 #154

Nyholm · 2020-12-19T09:56:04Z

This is the first steps to move towards what is described in https://github.com/Nyholm/next-faker

All classes/interfaces are marked with "Experimental". This PR does not mean that it is a perfect solution, but it is a step towards it.

src/Faker/Container/ContainerException.php

src/Faker/Container/NotInContainerException.php

src/Faker/Container/Container.php

src/Faker/Container/ContainerBuilder.php

localheinz · 2020-12-19T12:25:13Z

src/Faker/Container/ContainerBuilder.php

+        return new Container($this->container);
+    }
+
+    public static function getDefault(): ContainerInterface


Suggested change

public static function getDefault(): ContainerInterface

public static function create(): ContainerInterface

This could be confused with "build".

I want to be clear that this builds the default container.. Maybe this should be part of Generator or Factory instead?

I'm not even sure we need this class, to be honest.

I want the container to be immutable. Using a builder class is the simplest and easiest to understand.

The alternative would be that the container builds itself and it is immutable only after we run Container::build().

src/Faker/English/FileExtension.php

Nyholm

Thank you for the review

src/Faker/Container/Container.php

Nyholm · 2020-12-19T12:26:46Z

src/Faker/Container/Container.php

+            try {
+                return $this->cache[$id] = new $data();
+            } catch (\Throwable $e) {
+                throw new ContainerException(sprintf('Could not instantiate class "%s"', $id), 0, $e);
+            }


What is the benefit of this change?

Nyholm · 2020-12-19T12:37:45Z

Thank you for the review, The PR is updated and build is green.

I left you some questions

src/Faker/Excpetion/ExtensionNotFound.php

src/Faker/Extension/GeneratorAwareExtension.php

src/Faker/Generator.php

test/Faker/Container/ContainerBuilderTest.php

test/Faker/Container/ContainerTest.php

src/Faker/Container/Container.php

src/Faker/Container/ContainerBuilder.php

localheinz · 2020-12-19T12:40:11Z

src/Faker/English/FileExtension.php

+
+declare(strict_types=1);
+
+namespace Faker\English;


Not sure about the namespace, is an implementation for German going to be different?

I thought about this. I added it in this namespace to be constant with the German package.

The German package is called fakerphp/german, it lives in https://github.com/FakerPHP/German and its root namespace would be Faker\German.

So there will be a Faker\German\PersonDe and Faker\German\PersonAu..

However, The Australian English will not be in the root package. There should also be a fakerphp/english with Brittish, Australian... etc.

We currently have quite a few providers that are not necessarily language specific (besides this one), such as UserAgent or Barcode.
How about adding a dedicated package/namespace that provides those?

I have considered that and decided not to.

It is impossible of us to know if there is a specific Russian barcode or a Chinese UserAgent. So I think the core should treat all extensions the same. But I agree with you, there are only some that are likely to be localised.

Imo English should not be in the core package as well? Lets just put it in its own package.

Meaning people will require then locale extension and not the core directly?

I have considered that and decided not to.

It is impossible of us to know if there is a specific Russian barcode or a Chinese UserAgent. So I think the core should treat all extensions the same. But I agree with you, there are only some that are likely to be localised.

But arent we duplicating a lot this way? There are a lot of generic things so. Medical / Barcore / Number / Date etc

No. We are not duplicating.
Since the core package includes all English and “generic providers”, no language specific extensions need to duplicate anything.

I'm somewhat reminded of our conversation in Nyholm/next-faker#14.
These "generic providers" are in the fakerphp/english package or in the core? How do we differentiate between generic and "localizable"?

Why even put english in the core package. I think it is a benefit to keep it separated and just require it. Reason for this is that it is more easy to setup new locale extensions from a proper example repo

Imo English should not be in the core package as well? Lets just put it in its own package.

If so, a third party library will require fakerphp/faker and get an unusable library because it does not have any extension. So the third partly library will then require fakerphp/faker and fakerphp/english and the potential "win" for separating them is lost and we only caused confusion.

But arent we duplicating a lot this way? There are a lot of generic things so. Medical / Barcore / Number / Date etc

No, we are not. The fakerphp/german does not have to include a Medical extension, it will just keep using the default ones.

These "generic providers" are in the fakerphp/english package or in the core?

They are in the code.

How do we differentiate between generic and "localizable"?

Just for context (so I understand), I have a counter question: Why would you need to differentiate between generic and "localizable"?

Reason for this is that it is more easy to setup new locale extensions from a proper example repo

Im not sure this is true. Feel free to open a new issue discussion the pros and cons. Consider that 90% of the users will only use the english locale and also see the discussions on https://github.com/Nyholm/next-faker

I would also like to remind you about my PR description:

This PR does not mean that it is a perfect solution, but it is a step towards it.

I dont mean to turn down this discussion, but it is quite far from the purpose of this PR.

src/Faker/Generator.php

src/Faker/Container/Container.php

localheinz · 2020-12-20T09:52:35Z

src/Faker/Container/Container.php

+            }
+
+            try {
+                return $this->services[$id] = new $definition();


Writing code this way has the following downsides:

a reader does not know whether the intent is to compare or assign a value

return $this->services[$id] == new $definition();

it does not separate concerns

trying to create a value and catching an exception that can be potentially thrown

assigning a value

returning a value

it also hides whether we have tested both failure and success cases, this line of code will be covered even when we have only a test case for one of failure or success case

src/Faker/Container/ContainerBuilder.php

test/Faker/Container/ContainerTest.php

test/Faker/GeneratorTest.php

test/Faker/Container/ContainerTest.php

test/Faker/GeneratorTest.php

localheinz · 2020-12-20T11:58:11Z

@Nyholm

Probably best to rebase on top of `main!

src/Faker/English/FileExtension.php

localheinz · 2020-12-20T14:21:49Z

src/Faker/Container/Container.php

+    public function has($id)
+    {
+        if (!is_string($id)) {
+            throw new \LogicException(sprintf('First argument of %s::get() must be string', self::class));


Suggested change

throw new \LogicException(sprintf('First argument of %s::get() must be string', self::class));

throw new \InvalidArgumentException(sprintf('First argument of %s::get() must be string', self::class));

localheinz · 2020-12-20T18:02:41Z

src/Faker/Container/ContainerBuilder.php

+use Faker\Core\File;
+use Faker\Extension\FileExtension;


@Nyholm

Hm..

Faker\Extension\File is an interface. Faker\English\FileExtension is the implementation of that interface.

I could have named it Faker\English\File but that would be more confusing.

It is only confusing when we always import every single class:

diff --git a/src/Faker/Container/ContainerBuilder.php b/src/Faker/Container/ContainerBuilder.php index b86992b9..109f03ea 100644 --- a/src/Faker/Container/ContainerBuilder.php +++ b/src/Faker/Container/ContainerBuilder.php @@ -4,8 +4,8 @@ declare(strict_types=1); namespace Faker\Container; -use Faker\Core\File; -use Faker\Extension\FileExtension; +use Faker\Core; +use Faker\Extension; use Psr\Container\ContainerInterface; /** @@ -61,7 +61,7 @@ final class ContainerBuilder public static function defaultExtensions(): array { return [ - FileExtension::class => File::class + Extension\File::class => Core\File::class ]; }

…to extension

src/Faker/Extension/Container.php

src/Faker/Extension/FileExtension.php

Nyholm · 2020-12-21T19:27:52Z

Thank you for all the reviews, suggestions and opinions. I think this PR is way better now than a few days ago.

pimjansen · 2020-12-22T09:03:18Z

@localheinz im happy to take this in as is, what is your view? We can always change from there. Like @Nyholm said it is not a 100% perfect solution yet but the foundation of the future.

localheinz

👍

We can still reiterate when necessary!

Nyholm · 2020-12-22T09:32:20Z

Wohoo. Thank you for merging

* Adding support for extensions and PSR-11 * Added getDefaultExtensions * minor * Apply suggestions from code review Co-authored-by: Andreas Möller <am@localheinz.com> * CS fixes * Refactor container * CS * Renamed namespace * Updated names and namespaces * Fix: Run 'make cs' * Fix: Remove unnecessary initialization * Fix: Add missing throws annotation * Fix: Run 'make baseline' * Fix: Wrapping * Fix: Throw InvalidArgumentException * Fix: Run 'make cs' * Fix: Use elvis operator * Fix: Move exception to Extension namespace * Fix: Add return type declarations * Fix: Avoid unnecessary imports * Fix: Whitespace * Fix: Add covers annotation * Enhancement: Throw RuntimeException when service can not be resolved to extension * Fix: DocBlock * Fix: Run 'make cs' * Fix: Namespace * Enhancement: Merge Container into Extension namespace * Added dock block * cs Co-authored-by: Andreas Möller <am@localheinz.com>

Nyholm marked this pull request as ready for review December 19, 2020 10:30

localheinz reviewed Dec 19, 2020

View reviewed changes

src/Faker/Container/ContainerBuilder.php Outdated Show resolved Hide resolved

localheinz reviewed Dec 19, 2020

View reviewed changes

src/Faker/Container/ContainerBuilder.php Outdated Show resolved Hide resolved

localheinz reviewed Dec 19, 2020

View reviewed changes

src/Faker/English/FileExtension.php Outdated Show resolved Hide resolved

Nyholm commented Dec 19, 2020

View reviewed changes

localheinz reviewed Dec 19, 2020

View reviewed changes

Nyholm force-pushed the psr-11 branch 2 times, most recently from cb5e45e to 06c25bb Compare December 19, 2020 13:36

localheinz reviewed Dec 20, 2020

View reviewed changes

Nyholm force-pushed the psr-11 branch from e9236ad to a75ed53 Compare December 20, 2020 11:59

krsriq reviewed Dec 20, 2020

View reviewed changes

src/Faker/English/FileExtension.php Outdated Show resolved Hide resolved

This was referenced Dec 20, 2020

Use symfony/phpunit-bridge #163

Merged

Adding trait for GeneratorAwareExtension #165

Merged

localheinz reviewed Dec 20, 2020

View reviewed changes

localheinz force-pushed the psr-11 branch from 656cd7b to 388244a Compare December 20, 2020 19:02

localheinz self-assigned this Dec 20, 2020

localheinz added the enhancement New feature or request label Dec 20, 2020

localheinz force-pushed the psr-11 branch from e5edf57 to 38ef437 Compare December 20, 2020 19:17

localheinz marked this pull request as draft December 20, 2020 22:01

localheinz force-pushed the psr-11 branch from 1cc67cd to 35cae52 Compare December 20, 2020 22:02

krsriq mentioned this pull request Dec 21, 2020

Update file Nyholm/next-faker#15

Merged

localheinz removed their assignment Dec 21, 2020

localheinz marked this pull request as ready for review December 21, 2020 09:09

localheinz force-pushed the psr-11 branch from 35cae52 to 5db107d Compare December 21, 2020 12:20

localheinz self-assigned this Dec 21, 2020

localheinz force-pushed the psr-11 branch from e2e9b3c to 1099d96 Compare December 21, 2020 12:41

localheinz added 11 commits December 21, 2020 16:31

Fix: Wrapping

0989aba

Fix: Throw InvalidArgumentException

113e256

Fix: Run 'make cs'

cfc4577

Fix: Use elvis operator

35979fb

Fix: Move exception to Extension namespace

07958b5

Fix: Add return type declarations

177eb99

Fix: Avoid unnecessary imports

ceec1b3

Fix: Whitespace

472a13d

Fix: Add covers annotation

f5ce576

Enhancement: Throw RuntimeException when service can not be resolved …

cfebd67

…to extension

Fix: DocBlock

b2fdf73

localheinz force-pushed the psr-11 branch from 136b04f to b2fdf73 Compare December 21, 2020 15:31

localheinz added 3 commits December 21, 2020 16:33

Fix: Run 'make cs'

100634c

Fix: Namespace

f3fb2b3

Enhancement: Merge Container into Extension namespace

a76d997

localheinz assigned Nyholm and unassigned localheinz Dec 21, 2020

pimjansen reviewed Dec 21, 2020

View reviewed changes

src/Faker/Extension/Container.php Show resolved Hide resolved

src/Faker/Extension/Container.php Show resolved Hide resolved

src/Faker/Extension/FileExtension.php Show resolved Hide resolved

Added dock block

f1336f4

Nyholm marked this pull request as ready for review December 21, 2020 19:27

cs

0460bd5

localheinz approved these changes Dec 22, 2020

View reviewed changes

pimjansen merged commit 9cca954 into FakerPHP:main Dec 22, 2020

Nyholm deleted the psr-11 branch December 22, 2020 09:32

localheinz mentioned this pull request Dec 22, 2020

Fix: Add missing tests for ContainerBuilder #228

Merged

1 task

localheinz mentioned this pull request Sep 5, 2023

Fix: Mark trait as experimental #738

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding support for extensions and PSR-11 #154

Adding support for extensions and PSR-11 #154

Nyholm commented Dec 19, 2020

localheinz Dec 19, 2020

Nyholm Dec 19, 2020

localheinz Dec 20, 2020

Nyholm Dec 20, 2020

Nyholm left a comment

Nyholm Dec 19, 2020

Nyholm commented Dec 19, 2020

localheinz Dec 19, 2020

Nyholm Dec 19, 2020

krsriq Dec 19, 2020

Nyholm Dec 19, 2020

pimjansen Dec 19, 2020

pimjansen Dec 19, 2020

Nyholm Dec 19, 2020

krsriq Dec 19, 2020

pimjansen Dec 19, 2020

Nyholm Dec 20, 2020

localheinz Dec 20, 2020

localheinz commented Dec 20, 2020

localheinz Dec 20, 2020

localheinz Dec 20, 2020

Nyholm commented Dec 21, 2020

pimjansen commented Dec 22, 2020

localheinz left a comment

Nyholm commented Dec 22, 2020

	public static function getDefault(): ContainerInterface
	public static function create(): ContainerInterface

	throw new \LogicException(sprintf('First argument of %s::get() must be string', self::class));
	throw new \InvalidArgumentException(sprintf('First argument of %s::get() must be string', self::class));

Adding support for extensions and PSR-11 #154

Adding support for extensions and PSR-11 #154

Conversation

Nyholm commented Dec 19, 2020

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

Nyholm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nyholm commented Dec 19, 2020

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

localheinz commented Dec 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nyholm commented Dec 21, 2020

pimjansen commented Dec 22, 2020

localheinz left a comment

Choose a reason for hiding this comment

Nyholm commented Dec 22, 2020