Skip to content

Conversation

@Chris53897
Copy link
Contributor

@Chris53897 Chris53897 commented Oct 28, 2025

(since Symfony 8)

@Chris53897 Chris53897 marked this pull request as draft October 28, 2025 10:39
@Chris53897 Chris53897 marked this pull request as ready for review October 30, 2025 07:30
@Chris53897
Copy link
Contributor Author

@bytehead

I did update composer.json locally to test it with adding symfony 8 and i added

"minimum-stability": "beta",
"prefer-stable": false

Tests are green.

Copy link
Member

@bytehead bytehead left a comment

Choose a reason for hiding this comment

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

Looks good, just some CS nitpick ☺️

Copy link
Member

@bytehead bytehead left a comment

Choose a reason for hiding this comment

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

Thank you @Chris53897!

@bytehead bytehead changed the title feat: use PhpFileLoader if XmlFileLoader is not available Use php-based configs when xml is not available anymore Oct 31, 2025
@bytehead bytehead merged commit 3fdb122 into 1up-lab:main Oct 31, 2025
16 checks passed
@skmedix
Copy link

skmedix commented Oct 31, 2025

Quick question: what’s the point of keeping both XML and PHP configs? If both are supported, and there’s no BC issue with PHP config, and they’re functionally identical, why not just leave PHP config?

@Chris53897
Copy link
Contributor Author

The reason is that symfony 8 does not support xml-based configs anymore.
To support Symfony 5.4 to 8.x it is necessary to support xml and php based.
Once support for symfony 5.4 is dropped, the xml-based configs can be dropped.

@skmedix
Copy link

skmedix commented Nov 1, 2025

The reason is that symfony 8 does not support xml-based configs anymore. To support Symfony 5.4 to 8.x it is necessary to support xml and php based. Once support for symfony 5.4 is dropped, the xml-based configs can be dropped.

However, this does not answer my question. Symfony 5.4-8.0 supports PhpFileLoader, so in my opinion, there is no reason or benefit of keeping the XML config. By migrating to PHP config completely, we do not introduce a BC.

@Chris53897
Copy link
Contributor Author

@skmedix You are right. I was not aware that PhpFileLoader is available in Symfony 5.4. (Misleaded by an other PR)

I do not know how to rewrite this test, to use the PHP File instead of the .xml file.
https://github.com/1up-lab/OneupFlysystemBundle/blob/main/tests/DependencyInjection/OneupFlysystemExtensionTest.php#L95

@skmedix
Copy link

skmedix commented Nov 3, 2025

I do not know how to rewrite this test, to use the PHP File instead of the .xml file. main/tests/DependencyInjection/OneupFlysystemExtensionTest.php#L95

Try this:

public function testAdapterAvailability(): void
{
    $container = new ContainerBuilder();
    $loader = new PhpFileLoader($container, new FileLocator(__DIR__ . '/../../src/Resources/config'));
    $loader->load('adapters.php');

    foreach ($container->getDefinitions() as $id => $definition) {
        if ('service_container' === $id) {
            continue;
        }

        $class = $definition->getClass();
        if (null !== $class) {
            self::assertTrue(class_exists($class), 'Could not load class: ' . $class);
        }
    }
}

@Chris53897

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants