Skip to content

default_index_method no longer works with contiguous integers since 7.0 #60651

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
Bilge opened this issue Jun 3, 2025 · 10 comments
Closed

default_index_method no longer works with contiguous integers since 7.0 #60651

Bilge opened this issue Jun 3, 2025 · 10 comments
Labels
Bug DependencyInjection Status: Needs Review

Comments

@Bilge
Copy link
Contributor

Bilge commented Jun 3, 2025 •
edited
Loading

Symfony version(s) affected

7.2.7

Description

Worked fine in 6.4, but now it seems to be completely ignored in 7.x. The tagged locator just uses the class name as the key now, ignoring the value returned by default_index_method, despite the fact that the method is still called at container compile time. In case it matters, my index method returns in int.

How to reproduce

Tag some services.

services:
  _instanceof:
    Org\AbstractUnpacker:
      tags:
        - unpacker

Inject services.

services:
  Org\Unpacker:
    arguments:
      $unpackers: !tagged_locator
        tag: unpacker
        default_index_method: getVersion
namespace Org;

abstract class AbstractUnpacker
{
    abstract public static function getVersion(): int;
}

Possible Solution

No response

Additional Context

No response
@Bilge Bilge added the Bug label Jun 3, 2025
@Bilge Bilge changed the title default_index_method no longer works in 7.2 default_index_method no longer works in 7.2 Jun 3, 2025
@HypeMC
Copy link
Member

HypeMC commented Jun 3, 2025

@Bilge At first glance, your service definition seems to be incorrect. It should be:

_instanceof:
    Org\AbstractUnpacker:
        tags:
            - unpacker

I've tested it, and it seems to work as expected. Here's the dumped container:

public static function do($container, $lazyLoad = true)
{
    include_once \dirname(__DIR__, 4).'/src/Unpacker/Unpacker.php';

    return $container->privates['App\\Unpacker\\Unpacker'] = new \App\Unpacker\Unpacker(new \Symfony\Component\DependencyInjection\Argument\ServiceLocator($container->getService ??= $container->getService(...), [
        10 => ['privates', 'App\\Unpacker\\BarUnpacker', 'getBarUnpackerService', true],
        5 => ['privates', 'App\\Unpacker\\FooUnpacker', 'getFooUnpackerService', true],
    ], [
        10 => 'App\\Unpacker\\BarUnpacker',
        5 => 'App\\Unpacker\\FooUnpacker',
    ]));
}

Could you create a small example application to reproduce your issue?

@Bilge
Copy link
Contributor Author

Bilge commented Jun 3, 2025 •
edited
Loading

I am glad it works for you but clearly it does not (any longer) for me.

public static function do($container, $lazyLoad = true)
    {
        include_once \dirname(__DIR__, 4).'/src/CrankingService.php';

        if (isset($container->privates['Org\\CrankingService'])) {
            return $container->privates['Org\\CrankingService'];
        }

        return $container->privates['Org\\CrankingService'] = new Org\CrankingService(new \Symfony\Component\DependencyInjection\Argument\ServiceLocator($container->getService ??= $container->getService(...), [
            'Org\\UnpackerV0' => ['privates', 'Org\\UnpackerV0', 'getUnpackerV0Service', true],
            'Org\\UnpackerV1' => ['privates', 'Org\\UnpackerV1', 'getUnpackerV1Service', true],
        ], [
            'Org\\UnpackerV0' => 'Org\\UnpackerV0',
            'Org\\UnpackerV1' => 'Org\\UnpackerV1',
        ]));
    }

Since you already produced a small example to test it, perhaps you could share that so I can see what you've done differently (and modify if needs be)?

I notice you're using non-contiguous numbering (10, 5) rather than (0, 1). Perhaps there is code somewhere that checks if it looks like a list and does something different...

@HypeMC
Copy link
Member

HypeMC commented Jun 3, 2025

Since you already produced a small example to test it, perhaps you could share that so I can see what you've done differently (and modify if needs be)?

@Bilge Here you go: 7.2-default-index-method.zip. There's a TestCommand that uses the services. Hope it helps.

I notice you're using non-contiguous numbering (10, 5) rather than (0, 1). Perhaps there is code somewhere that checks if it looks like a list and does something different...

Not sure, could you perhaps test that case?

@Bilge
Copy link
Contributor Author

Bilge commented Jun 3, 2025 •
edited
Loading

I don't know how you can claim it "works". Without making any changes whatsoever to the test application you provided, running the command you included fails with:

❯ bin/console app:test -v

In ServiceLocator.php line 137:

  [Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException]
  Service "5" not found: the container inside "Symfony\Component\DependencyInjection\Argument\ServiceLocator" is a smaller service locator that only knows about the "App\Unpacker\FooUnpacker" and "App\Unpacker\BarUnpacker" services.


Exception trace:
  at D:\temp\sfbug\vendor\symfony\dependency-injection\ServiceLocator.php:137
 Symfony\Component\DependencyInjection\ServiceLocator->createNotFoundException() at D:\temp\sfbug\vendor\symfony\service-contracts\ServiceLocatorTrait.php:48
 Symfony\Component\DependencyInjection\ServiceLocator->doGet() at D:\temp\sfbug\vendor\symfony\dependency-injection\ServiceLocator.php:43
 Symfony\Component\DependencyInjection\ServiceLocator->get() at D:\temp\sfbug\vendor\symfony\dependency-injection\Argument\ServiceLocator.php:34
 Symfony\Component\DependencyInjection\Argument\ServiceLocator->get() at D:\temp\sfbug\src\Unpacker\Unpacker.php:17
 App\Unpacker\Unpacker->foo() at D:\temp\sfbug\src\Command\TestCommand.php:25
 App\Command\TestCommand->execute() at D:\temp\sfbug\vendor\symfony\console\Command\Command.php:279
 Symfony\Component\Console\Command\Command->run() at D:\temp\sfbug\vendor\symfony\console\Application.php:1094
 Symfony\Component\Console\Application->doRunCommand() at D:\temp\sfbug\vendor\symfony\framework-bundle\Console\Application.php:123
 Symfony\Bundle\FrameworkBundle\Console\Application->doRunCommand() at D:\temp\sfbug\vendor\symfony\console\Application.php:342
 Symfony\Component\Console\Application->doRun() at D:\temp\sfbug\vendor\symfony\framework-bundle\Console\Application.php:77
 Symfony\Bundle\FrameworkBundle\Console\Application->doRun() at D:\temp\sfbug\vendor\symfony\console\Application.php:193
 Symfony\Component\Console\Application->run() at D:\temp\sfbug\vendor\symfony\runtime\Runner\Symfony\ConsoleApplicationRunner.php:49
 Symfony\Component\Runtime\Runner\Symfony\ConsoleApplicationRunner->run() at D:\temp\sfbug\vendor\autoload_runtime.php:29
 require_once() at D:\temp\sfbug\bin\console:15

app:test

This is exactly the problem I was posting about, so again, not sure how it works for you (but thanks for the reproducer).

@HypeMC
Copy link
Member

HypeMC commented Jun 3, 2025

@Bilge Ok, so now I'm confused, did you change anything on your side?

$ bin/console app:test
1 App\Unpacker\FooUnpacker {#248}
2 App\Unpacker\BarUnpacker {#331}

@HypeMC
Copy link
Member

HypeMC commented Jun 3, 2025

@Bilge Which PHP version are you using?

@Bilge
Copy link
Contributor Author

Bilge commented Jun 3, 2025

For the above test:

PHP 8.4.1 (cli) (built: Nov 20 2024 11:13:29) (ZTS Visual C++ 2022 x64)

For the original report:

PHP 8.4.5 (cli) (built: Mar 14 2025 00:11:52) (NTS)

@MatTheCat
Copy link
Contributor

@HypeMC it indeed is caused by the indexes making the references a list..!

symfony/src/Symfony/Component/DependencyInjection/Compiler/ServiceLocatorTagPass.php

Lines 113 to 137 in c1d73b1

public static function map(array $services): array
{
$i = 0;
foreach ($services as $k => $v) {
if ($v instanceof ServiceClosureArgument) {
continue;
}
if ($i === $k) {
if ($v instanceof Reference) {
unset($services[$k]);
$k = (string) $v;
}
++$i;
} elseif (\is_int($k)) {
$i = null;
}
$services[$k] = new ServiceClosureArgument($v);
}
return $services;
}
}

@Bilge
Copy link
Contributor Author

Bilge commented Jun 3, 2025

@Bilge Ok, so now I'm confused, did you change anything on your side?

$ bin/console app:test
1 App\Unpacker\FooUnpacker {#248}
2 App\Unpacker\BarUnpacker {#331}

I did initially change them to 0, 1 but then I reverted my changes and the problem persisted. In case that somehow matters.

@MatTheCat
Copy link
Contributor

Looks like the change comes from #50578 which slightly modified the ServiceLocatorTagPass. Does that ring a bell @alexandre-daubois?

@Bilge Bilge changed the title default_index_method no longer works in 7.2 default_index_method no longer works with contiguous integers since 7.0 Jun 3, 2025
@MatTheCat MatTheCat mentioned this issue Jun 4, 2025
Merged
nicolas-grekas added a commit that referenced this issue Jun 6, 2025
@nicolas-grekas
bug #60691 [DependencyInjection] Fix ServiceLocatorTagPass indexes …
16efe96 
…handling (MatTheCat)

This PR was merged into the 7.2 branch.

Discussion
----------

[DependencyInjection] Fix `ServiceLocatorTagPass` indexes handling

| Q             | A
| ------------- | ---
| Branch?       | 7.2
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | Fix #60651
| License       | MIT

#50578 changed the `ServiceLocatorTagPass`’ behavior so that if the map is a list, its indexes always are replaced by the corresponding reference ID, even if they come from the `PriorityTaggedServiceTrait`. That means if your service map ends up as a list because e.g. your `default_index_method` returned contiguous integer, its indexes will be replaced.

This PR reverts this change so that it works the same than v6.

Commits
-------

a73c9d1 [DependencyInjection] Fix `ServiceLocatorTagPass` indexes handling
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug DependencyInjection Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@nicolas-grekas @Bilge @MatTheCat @xabbuh @HypeMC @carsonbot