Dev into master #198

Danack · 2023-04-20T13:53:54Z

Everything should be just fixes, unless I've mucked up.

At Levi's suggestion, PHP 7.2.0 is the new minimum version as that is the version supported by Ubuntu LTS.

… run on, as the PHP exception message changed.

…hroughAlias.

…robably appropriate.

…injector->make()/excute() args.

…a better place. Also avoids the need for an error suppression usage.

…ated.

baohx2000 · 2023-05-17T19:49:26Z

Looks like something broke 7.2 and 7.3 compatibility, but I personally don't think they should be supported anymore, regardless of if they are part of a LTS distribution. 7.4 is the oldest supported (security fixes only) version of PHP at this time.

baohx2000

oof, that was a chonky pr.

Danack · 2023-05-23T10:18:21Z

Looks like something broke 7.2 and 7.3 compatibility,

Only the benchmark. I've split that off into a separate job.

morrisonlevi · 2023-05-31T14:56:38Z

test/fixtures_8_1.php

+
+
+
+


This is about 4 blank lines too many 😆

morrisonlevi

I have not reviewed the whole thing, but as a generalization, it looks good to me.

I want affirm that I think for a low-maintenance project like Auryn, having wide PHP version support is a virtue, not a hindrance. Thank you for supporting PHP 7.2.

vlakarados · 2023-07-26T14:11:54Z

Just stopped by to say thank you for supporting this. Still using auryn in dozens of productions and still loving it.

kelunik · 2023-07-29T17:28:57Z

.github/workflows/ci.yml

+          ini-values: memory_limit=-1
+
+      - name: "Add PHPUnit matcher"
+        run: echo "::add-matcher::${{ runner.tool_cache }}/phpunit.json"


What does this do?

Guessing here, but since we execute simple-phpunit instead of running phpunit directly, GH actions doesn't know how to map errors back to code to show properly in the action results. This fixes that by referring to the gh-action built-in mapping configuration for phpunit. https://fusectore.dev/2021/11/19/github-action-problem-matchers.html

kelunik · 2023-07-29T17:29:54Z

.github/workflows/ci.yml

+      - name: "Add PHPUnit matcher"
+        run: echo "::add-matcher::${{ runner.tool_cache }}/phpunit.json"
+
+      - run: composer install


There should probably be a name here, too?

kelunik · 2023-07-29T17:35:30Z

README.md

+
+simple-phpunit accepts PHPUnit commandline options and passes them through to PHPUnit e.g. `php php vendor/bin/simple-phpunit --group wip` to only run the tests tagged as being part of group 'wip'.
+
+#### Running benchamarks


Suggested change

#### Running benchamarks

#### Running benchmarks

kelunik · 2023-07-29T17:36:05Z

composer.json

+        "friendsofphp/php-cs-fixer" :  "3.3.1",
+        "phpbench/phpbench": "1.1.1",


Do we want fixed versions here or ^ as well?

kelunik · 2023-07-29T17:37:01Z

lib/CachingReflector.php

@@ -52,7 +52,7 @@ public function getCtorParams($class)
        return $reflectedCtorParams;
    }

-    public function getParamTypeHint(\ReflectionFunctionAbstract $function, \ReflectionParameter $param)
+    public function getParamType(\ReflectionFunctionAbstract $function, \ReflectionParameter $param)


This is a BC break.

kelunik · 2023-07-29T17:42:08Z

lib/Injector.php

+
+

Suggested change

kelunik · 2023-07-29T17:43:59Z

lib/Reflector.php

@@ -40,7 +40,7 @@ public function getCtorParams($class);
     * @param \ReflectionFunctionAbstract $function
     * @param \ReflectionParameter $param
     */
-    public function getParamTypeHint(\ReflectionFunctionAbstract $function, \ReflectionParameter $param);
+    public function getParamType(\ReflectionFunctionAbstract $function, \ReflectionParameter $param);


This is a BC break.

kelunik · 2023-07-29T17:47:31Z

test/InjectorTest.php

    {
+        $this->expectException(\Auryn\InjectionException::class);
+        // TODO - why does this message end with double-colon?


Is this still an open TODO?

kelunik · 2023-07-29T17:48:10Z

test/InjectorTest.php

     */
    public function testCustomDefinitionNotPassedThrough()
    {
        $injector = new Injector();
        $injector->share('Auryn\Test\DependencyWithDefinedParam');
+
+        $this->expectException(\Auryn\InjectionException::class);
+        // TODO - why does this message end with double-colon?


Is this still an open TODO?

kelunik · 2023-07-29T17:50:12Z

test/fixtures_5_6.php

@@ -2,7 +2,7 @@

 namespace Auryn\Test;

-class NoTypehintNoDefaultConstructorVariadicClass


This file should likely be renamed if we drop 5.6 support.

vlakarados · 2023-08-26T18:41:53Z

Considering this repository is not getting too much love lately, does anyone have any concerns at this point in time in making all private methods protected to allow extending it? I have a couple of modifications on my mind that would require that, otherwise it's copy-pasting the entire \Auryn\Injector class.

Things I have in mind are Attribute support (One for sharing and one for delegating object instantiations) and the other one being adding additional debug information. With that said I'm pretty sure this is not something that would see it's way to master branch here considering the pretty conservative approach.

I'm sure there's many people who would come up with some additional ideas that would not get broken on updates of this repository.

Danack · 2023-08-27T13:50:57Z

With that said I'm pretty sure this is not something that would see it's way to master branch here considering the pretty conservative approach.

Would there be any downside in making the methods be protected by default?

I mean, other than possibly people asking for more changes later.

baohx2000 · 2023-08-27T16:03:46Z

With the BC break mentioned above and just the sheer size of this change set, maybe it's time to just call it 2.0-beta just to get it merged? I personally have no issue with changing some methods to protected, especially if it means easily pluggable Attribute support.

vlakarados · 2023-08-27T17:27:21Z

With that said I'm pretty sure this is not something that would see it's way to master branch here considering the pretty conservative approach.

Would there be any downside in making the methods be protected by default?

I mean, other than possibly people asking for more changes later.

I would say no. From the "your average developer" perspective, if you can't do something you go for a dirty workaround. Considering the change from private to protected is not related to the userland code (as in using Auryn for it's defined functionality in public methods), but rather for extension purposes - to improve the library by additional functionality, I don't see any reason not to.

Just a simple and quite popular example in the issues section is the PSR-11 container interop compatibility, which to my mind is a no-go to be in this library because of the service locator as mentioned multiple times, but let's drop down to earth - this is often a requirements for integrating other packages. Then you end up with 2 of them, which reveals a question of "why 2, when we can have one (not auryn) as the code is already polluted with service location.
But where am I leading is this: https://github.com/elazar/auryn-container-interop - probably most well known and used extension, but looking at the code it's "hacky" because of reusing what is meant to be an array of class strings as keys gets reused for arbitrary strings. As it's a service locator, I bet it should be share-by-default in 99% of the cases as well.

I mean, a much better implementation would be possible.

Agree with what @baohx2000 said, it can be a v2, even with BC breaks. This might even be a better approach as it will be easier to maintain this.

nevez · 2023-09-07T09:56:45Z

Hello, I'm about to start a new PHP project using auryn. Having seen the amount of changes of this pull request and the possibility that some of them might break compatibility, do you recommend using this version instead of the stable (master) one even if it hasn't been merged yet? Apart from bug fixes and modern php support, are there changes in funcionality? Thank you

martin-hughes and others added 30 commits December 9, 2022 17:05

Add notice pointing at new repos.

d1bd25a

Updated words to be accurate for Dec 2022.

283779d

Updated to modern version of PHPUnit and add phpunit switcher for CI.

57a4275

Dynamic property creation is no longer a thing.

93920b0

Change config to avoid warnings on coverage only tests.

0460307

Relax access for properties so that they can be read by tests.

c254996

Ignore files that should never be added.

c911e18

Migrated PHPUnit config file to modern format.

15d947b

Fix majority of tests. Remaining ones are more difficult.

1e59a90

Fix two mocking errors.

2ad6fc3

Skip tests that need proper investigation.

e57ba16

Fix test testDelegationDoesntMakeObject

c6709a7

Continue running in CI on failures for now.

d01a31a

Try removing actual phpunit to see if it makes tests run.

23274ea

Remove old annotations as they are giving deprecation warnings.

f6d498e

Double-check having phpunit breaks CI.

321aae3

Update badge to my repo for now.

b2fbae6

Make name easier to understand.

067bc79

Investigate when behaviour changed.

0f3b07f

Remove phpunit again, it is causing a problem as a direct dependency.

aff99a5

Adjust tests expectation based on which version of PHP they are being…

8b46ab0

… run on, as the PHP exception message changed.

Correct typos.

ae2dee4

Fix spelling to work on PHP 7.x - might still be broken on 8+

b7771b8

Hack fix for InjectorTest::testNonConcreteDependencyWithDefaultValueT…

d037f7d

…hroughAlias.

Unskip other tests that are now working.

7ecc162

Drop support for PHP < 7.3

874b26b

fix #129

113f8c7

Add PHP 8.2 to testing.

1f8809a

Change test rather than changing code...technically a BC break, but p…

76070d0

…robably appropriate.

Run CI against PHP 7.2

95067d2

Danack added 16 commits April 17, 2023 12:34

Added tests and improved error messages, and wrote documentation for …

e00a69b

…injector->make()/excute() args.

Added note on static method of how it could be private.

65a1517

Added phpbench config file.

83592b6

Ignore lines for code coverage, so that testing can be at 100% locally.

cef6506

Document behaviour of prepare replacement.

829fd95

Move check for whether an object was created by a delegate method to …

12e8f2c

…a better place. Also avoids the need for an error suppression usage.

Prevent double sharing of same type.

2d03a9e

Add test coverage for new in initializer.

f2f81c2

Added test coverage and note on why optional dependencies are not cre…

f7cb772

…ated.

Fix property name.

46b7998

Fix trailing comma.

322c91d

Minimum version is PHP 7.2 which is > 5.6 so check is not needed.

ae0e21e

Adding non-trivial benchmark.

e3e9d00

WIP words...

64f32c4

Reasonable draft of words.

5a3e751

Typos.

4cd4b4e

baohx2000 approved these changes May 18, 2023

View reviewed changes

Move benchmarks to their own job, and only run on two versions of PHP.

b739eb9

Danack assigned kelunik May 23, 2023

morrisonlevi reviewed May 31, 2023

View reviewed changes

test/fixtures_8_1.php

Comment on lines +13 to +16

Copy link

Collaborator

morrisonlevi May 31, 2023

Choose a reason for hiding this comment

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

This is about 4 blank lines too many 😆

morrisonlevi reviewed May 31, 2023

View reviewed changes

kelunik reviewed Jul 29, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dev into master #198

Dev into master #198

Danack commented Apr 20, 2023

baohx2000 commented May 17, 2023

baohx2000 left a comment

Danack commented May 23, 2023

morrisonlevi May 31, 2023

morrisonlevi left a comment

vlakarados commented Jul 26, 2023

kelunik Jul 29, 2023

baohx2000 Aug 27, 2023

kelunik Jul 29, 2023

kelunik Jul 29, 2023

kelunik Jul 29, 2023

kelunik Jul 29, 2023

kelunik Jul 29, 2023

kelunik Jul 29, 2023

kelunik Jul 29, 2023

kelunik Jul 29, 2023

kelunik Jul 29, 2023

vlakarados commented Aug 26, 2023

Danack commented Aug 27, 2023

baohx2000 commented Aug 27, 2023

vlakarados commented Aug 27, 2023

nevez commented Sep 7, 2023


		simple-phpunit accepts PHPUnit commandline options and passes them through to PHPUnit e.g. `php php vendor/bin/simple-phpunit --group wip` to only run the tests tagged as being part of group 'wip'.

		#### Running benchamarks

		"friendsofphp/php-cs-fixer" : "3.3.1",
		"phpbench/phpbench": "1.1.1",

		@@ -2,7 +2,7 @@

		namespace Auryn\Test;

		class NoTypehintNoDefaultConstructorVariadicClass

Dev into master #198

Are you sure you want to change the base?

Dev into master #198

Conversation

Danack commented Apr 20, 2023

baohx2000 commented May 17, 2023

baohx2000 left a comment

Choose a reason for hiding this comment

Danack commented May 23, 2023

Choose a reason for hiding this comment

morrisonlevi left a comment

Choose a reason for hiding this comment

vlakarados commented Jul 26, 2023

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

vlakarados commented Aug 26, 2023

Danack commented Aug 27, 2023

baohx2000 commented Aug 27, 2023

vlakarados commented Aug 27, 2023

nevez commented Sep 7, 2023