Failing tests, nullable param types, package maintenance?

[dlgoodchild]

I've just setup the package to make a PR and/or a suggestion, however having run the tests, I get:

...EE...E..R..EEEE.REERE.EEE...E.EEE..EE.....................RE  63 / 120 ( 52%)
..........EEEEEE.FFFE...EEEEEEEER....E....RR....EEE...E.E       120 / 120 (100%)

That's a lot of failures before even getting started. Is this expected? If so, my next question is, is this package actually maintained or is it largely abandoned now? I don't particularly want to fork and become the maintainer, so I'm just deciding on how I move forward with this. It's a very good package and would be a shame to see it abandoned.

There seem to be issues around nullable types e.g. ?SomeClass $sc. I also find the default value behaviour unexpected. I.e. if I have a SomeService(?MaybeService $mc = null), then this is always null, even if MaybeService could be autoloaded and supplied. I personally worked to the idea that this would be provided if it could be found, and if not, then it uses the default, seems natural, at least to us. So I'd propose it to work that way.

Anyway, the first thing I was going to do was fix the nullable issue. Then after that I was going to address the scenario I've just mentioned, however if that's against your design, then I'll just fork and go on from there, no problem. But before launching into fixing tests and opening PRs I first want to see if this is currently, or plans to be, abandoned, and what the appetite is for change.

Thanks.

[dlgoodchild]

A lot of the test failures are just that the tests are written against PHPUnit v4. Any objection to tests being upgraded to PHPUnit 9, at least?

Also, I see this is written to support PHP 5.6, maybe there's a moment to archive off a release for particular PHP versions and start supporting the latest as PHP is moving along quite a bit now in terms of syntax and support etc.

[kelunik]

Yes, please do a PR, but on https://github.com/amphp/auryn instead. We'll do a new major there including a minimum PHP version upgrade.

[kelunik]

I'm working on the phpunit upgrade now, so please don't start so we don't do double work.

[dlgoodchild]

I've already done a pretty big chunk of it against PHPUnit v9, and I've also added a docker container (v8) to make it easier for anyone to run the tests without needing PHP or composer installed, etc. It's no problem cos either way I need to make these changes and a passing test suite is the first step.

[kelunik]

I'm also pretty much done now. :-/

[kelunik]

I have 5 tests or so left that are currently failing after migration, they return null where the injector should provide a value, currently no clue why.

[dlgoodchild]

I have similar failures around null (though I have 7 risky as well as there are no asserts in those tests). So, taking:

class NonConcreteDependencyWithDefaultValue
{
    public $interface;
    public function __construct(DelegatableInterface $i = null)
    {
        $this->interface = $i;
    }
}

This is actually the behaviour I'm seeing in my project and needing to fix. When there is a default, I always seem to get the default, not the known interface or concrete.

[kelunik]

Which version of PHP do you use?

[dlgoodchild]

PHP 8 (via docker)

[kelunik]

@dlgoodchild Found the mistake, normalizeName needs to ltrim ?, as those parameters are now nullable.

[dlgoodchild]

@kelunik I've just started back onto this again. Yes I agree that's the change required. Having now deep dived the code to understand exactly what it's doing, it feels like it could do with some refactoring. Also based on the code syntax used, it's currently minimum PHP 7.4. I'd actually be tempted to create a major release version with a minimum of PHP 8.0 and clean it up some at the same time, that's currently my thinking.

My test output is currently:

$ docker run --rm -v $(pwd):/srv auryn-php-cli                        
Loading composer repositories with package information
Updating dependencies
Nothing to modify in lock file
Installing dependencies from lock file (including require-dev)
Nothing to install, update or remove
Generating autoload files
26 packages you are using are looking for funding.
Use the `composer fund` command to find out more!
PHPUnit 9.5.2 by Sebastian Bergmann and contributors.

Warning:       No code coverage driver available

...........R.......R..R......................................R.  63 / 120 ( 52%)
................................R.........RR.............       120 / 120 (100%)

Time: 00:00.342, Memory: 6.00 MB

There were 7 risky tests:

1) Auryn\test\InjectorTest::testMakeInstanceStoresShareIfMarkedWithNullInstance
This test did not perform any assertions

/srv/test/InjectorTest.php:108

2) Auryn\test\InjectorTest::testTypelessDefineForAliasedDependency
This test did not perform any assertions

/srv/test/InjectorTest.php:202

3) Auryn\test\InjectorTest::testMakeInstanceHandlesNamespacedClasses
This test did not perform any assertions

/srv/test/InjectorTest.php:258

4) Auryn\test\InjectorTest::testInterfaceFactoryDelegation
This test did not perform any assertions

/srv/test/InjectorTest.php:583

5) Auryn\test\InjectorTest::testShareNewAlias
This test did not perform any assertions

/srv/test/InjectorTest.php:887

6) Auryn\test\InjectorTest::testDelegateClosure
This test did not perform any assertions

/srv/test/InjectorTest.php:992

7) Auryn\test\InjectorTest::testCloningWithServiceLocator
This test did not perform any assertions

/srv/test/InjectorTest.php:1000

OK, but incomplete, skipped, or risky tests!
Tests: 120, Assertions: 202, Risky: 7.

The commit for all that is over at: https://github.com/dlgoodchild/auryn/commit/b94ca6ef5a78c0e285b612174a6ff6330f6ab80f

[kelunik]

@dlgoodchild Sorry for not providing an update. I've already fixed that one and did some refactorings on https://github.com/amphp/injector. Should we move discussion there based on new issues?

[dlgoodchild]

For me this issue I opened hadn't yet been addressed. That said, I've literally just found and fixed the bug though. It was specifically when using string based shares and it not then making that class but instead passing in the default.

Commit: https://github.com/dlgoodchild/auryn/commit/f0d1260b81bdae1d07cb482f4949af89b07917e4

[dlgoodchild]

Have a read though my changes and let me know if you want it as a PR.

[martin-hughes]

If you don't fancy switching to the AMPHP Injector for whatever reason, #194 offers a package that I will endeavor to keep maintained.

(I realise this is probably quite late to the party!)

[Danack]

There seem to be issues around nullable types e.g. ?SomeClass $sc. I also find the default value behaviour unexpected.

Although that's not an unreasonable expectation, expecting the other way round is also not unreasonable.

As there would be a couple of small issues with implementing your expected solution, and it would be an annoying BC break, I think we'll leave it as is. I wrote some words in f7cb77243c77ad10954a34aba84a1dfe8795ec00 that I will put here also:

Optional aka null default parameters

Original discussion: https://github.com/rdlowrey/auryn/issues/190

Description

When a dependency is optional and/or nullable, then Auryn will always pass null as the parameter:

class NullableDependency {}
class DependsOnNullableDependency
{
    public ?NullableDependency $string;
    public function __construct(?NullableDependency $instance = null)
    {
        $this->instance = $instance;
    }
}

Some people might expect that the NullableDependency would be created.

Why the parameter is always null

• Auryn defaults to explicit configuration. If an dependency is optional, then defaulting to not creating it, is more consistent with that principle.

• Some classes are not instantiable e.g. abstract classes, or classes with uncreatable dependencies. If Auryn defaulted to attempting to create the type, we would need to add "don't create this class" rules/config.

Alternative solution - be explicit

$injector->alias(NullableDependency::class, NullableDependency::class);

Alternative solution - new in the initializer

As of PHP 8.1, you can use create objects the initializer:

class NullableDependency {}
class DependsOnNullableDependency
{
    public ?NullableDependency $string;
    public function __construct(?NullableDependency $instance = new NullableDependency)
    {
        $this->instance = $instance;
    }
}

[Danack]

If so, my next question is, is this package actually maintained or is it largely abandoned now?

It was working well enough for me....as it took me quite a while to get round to using PHP >= 8 due to a huge chronic pain problem. The other maintainers are also a little distracted.

I don't particularly want to fork and become the maintainer, ... It's a very good package and would be a shame to see it abandoned.

Behold! the tragedy of open source! People can think a package is important and shouldn't be abandoned....but don't want to maintain it themselves.

How about sponsoring me instead then? https://github.com/sponsors/Danack/

[dlgoodchild]

Hi @Danack

I've now moved on from PHP, the language is falling behind and has little to no pace with the departure of key contributors. I've always been happy to support, contribute and share when it comes to OSS. Contributing and being the package maintainer are two different things though, and I'm simply being open and honest that I'm happy to put the work in, contribute, but not to be the package owner. In actual fact, With the changes I made to the package, I simply just ran with that version for the years I depended on it.

Thank you to whomever is the owner, maintainer for the package (I say this, because it's just not at all clear); I stand by my comments, it's a great package and allowed me to build an entire framework around it.

I'm going to close this issue and the related PR as they no longer apply with having moved on from PHP.

Thanks.

[Danack]

Cool, no worries mate.

the language is falling behind and has little to no pace with the departure of key contributors.

Somewhat comically, people are also complaining about the language changing too much...