search

aws / aws-sdk-php-symfony

Apache License 2.0
350 stars 89 forks source link

As initial service config is in YAML format `symfony/yaml` should be required package #91

Open kaznovac opened 1 year ago

kaznovac commented 1 year ago

Describe the bug

if the project does not require the symfony/yaml app with this bundle will fail to boot

Expected Behavior

one of these

Current Behavior

exception thrown on app/kernel boot

Reproduction Steps

create symfony starter project configure it to use php configuration remove symfony/yaml install this bundle

Possible Solution

dev can add the symfony/yaml (even if the app itself has no need for it)

Additional Information/Context

No response

SDK version used

bundle: 2.5.0 sdk: not explicitly specified (resolved by composer)

Environment details (OS name and version, etc.)

N/A

RanVaknin commented 4 weeks ago

Hi @kaznovac ,

I'm not sure I understand the problem in here.

In my reproduction steps, it is symphony itself that is relying on symphony/ yaml, not the SDK.

For example:

$ symfony new my_symphony_project --full
* Creating a new Symfony project with Composer

 [WARNING] The --full flag is deprecated, use --webapp instead.                                                         

* Setting up the project under Git version control
  (running git init /Users/rvaknin/test_folder/my_symphony_project)

 [OK] Your project is now ready in /Users/rvaknin/test_folder/my_symphony_project                                       

$ composer create-project symfony/website-skeleton my_symphony_project 
Creating a "symfony/website-skeleton" project at "./my_symphony_project"

In CreateProjectCommand.php line 371:

  Project directory "/Users/rvaknin/test_folder/my_symphony_project" is not empty.  

create-project [-s|--stability STABILITY] [--prefer-source] [--prefer-dist] [--prefer-install PREFER-INSTALL] [--repository REPOSITORY] [--repository-url REPOSITORY-URL] [--add-repository] [--dev] [--no-dev] [--no-custom-installers] [--no-scripts] [--no-progress] [--no-secure-http] [--keep-vcs] [--remove-vcs] [--no-install] [--no-audit] [--audit-format AUDIT-FORMAT] [--ignore-platform-req IGNORE-PLATFORM-REQ] [--ignore-platform-reqs] [--ask] [--] [<package> [<directory> [<version>]]]

$ cd my_symphony_project 

$ ls config
bundles.php   packages      preload.php   routes        routes.yaml   services.yaml
$ php bin/console debug:container

PHP Deprecated:  ini_set(): assert.warning INI setting is deprecated in /Users/rvaknin/test_folder/my_symphony_project/vendor/symfony/runtime/Internal/BasicErrorHandler.php on line 35

Deprecated: ini_set(): assert.warning INI setting is deprecated in /Users/rvaknin/test_folder/my_symphony_project/vendor/symfony/runtime/Internal/BasicErrorHandler.php on line 35

Symfony Container Services
==========================

 ------------------------------------------------------------------------------------ -------------------------------------------------------------------------------------------- 
  Service ID                                                                           Class name                                                                                  
 ------------------------------------------------------------------------------------ -------------------------------------------------------------------------------------------- 
  App\Kernel                                                                           alias for "kernel"                                                                          
  Doctrine\Bundle\DoctrineBundle\Controller\ProfilerController                         Doctrine\Bundle\DoctrineBundle\Controller\ProfilerController                                
  Doctrine\Bundle\DoctrineBundle\Dbal\ManagerRegistryAwareConnectionProvider           Doctrine\Bundle\DoctrineBundle\Dbal\ManagerRegistryAwareConnectionProvider                  
  Doctrine\Common\Persistence\ManagerRegistry                                
  ....
  ....
  .... more results

Removing symphony / yaml causes an error with symphony itself (the SDK has not been added yet):

$ composer remove symfony/yaml

./composer.json has been updated
Running composer update symfony/yaml
Loading composer repositories with package information
Updating dependencies
Lock file operations: 0 installs, 0 updates, 1 removal
  - Removing symfony/yaml (v6.1.11)
Writing lock file
Installing dependencies from lock file (including require-dev)
Package operations: 0 installs, 0 updates, 1 removal
  - Removing symfony/yaml (v6.1.11)
Generating optimized autoload files
105 packages you are using are looking for funding.
Use the `composer fund` command to find out more!

Run composer recipes at any time to see the status of your Symfony recipes.

Executing script cache:clear [KO]
 [KO]
Script cache:clear returned with error code 1
!!  
!!  Deprecated: ini_set(): assert.warning INI setting is deprecated in /Users/rvaknin/test_folder/my_symphony_project/vendor/symfony/runtime/Internal/BasicErrorHandler.php on line 35
!!  PHP Deprecated:  ini_set(): assert.warning INI setting is deprecated in /Users/rvaknin/test_folder/my_symphony_project/vendor/symfony/runtime/Internal/BasicErrorHandler.php on line 35
!!  
!!  In FileLoader.php line 172:
!!                                                                                 
!!    Unable to load YAML config files as the Symfony Yaml Component is not insta  
!!    lled in /Users/rvaknin/test_folder/my_symphony_project/config/packages/cach  
!!    e.yaml (which is being imported from "/Users/rvaknin/test_folder/my_symphon  
!!    y_project/src/Kernel.php").                                                  
!!                                                                                 
!!  
!!  In YamlFileLoader.php line 752:
!!                                                                                 
!!    Unable to load YAML config files as the Symfony Yaml Component is not insta  
!!    lled.                                                                        
!!                                                                                 
!!  
!!  
Script @auto-scripts was called via post-update-cmd

Can you please write more specific repro steps that highlight the limitation you are facing?

Regarding your PR, we cannot change the default configuration from Yaml to XML, because current customers are likely relying on this current implementation and that change would not be backwards compatible.

Thanks, Ran~

kaznovac commented 3 weeks ago

Hi @RanVaknin, please note that you shouldn't use --full option when creating the new project. it will install the whole symfony framework - this is officially discouraged since sf 3.3 (may 2017); the recommended way is to install the needed components only (see https://symfony.com/doc/3.x/setup/flex.html) this is the reason removing symfony/yaml fails - it's is not explicitly required in your project (yes the project has it as whole symfony mono-repo is used as dependency)

Reproduction Steps

create symfony starter project configure it to use php configuration remove symfony/yaml install this bundle

  1. create the app

    [WARNING] The --full flag is deprecated, use --webapp instead.

as per your output you may use as suggested symfony new my_symphony_project --webapp but it's even better to do the simpler symfony new my_symphony_project as there will be less packages and configuration installed - this is will ease the second step (see https://symfony.com/doc/current/setup.html#creating-symfony-applications)

  1. reconfigure the app to use php config (see https://symfony.com/doc/current/configuration.html#using-php-configbuilders)

  2. now you can remove symfony/yaml by composer remove symfony/yaml

  3. install the bundle composer require aws/aws-sdk-php-symfony

  4. investigate and fix the bundle

Regarding your PR, we cannot change the default configuration from Yaml to XML, because current customers are likely relying on this current implementation and that change would not be backwards compatible.

sorry but this is not correct - the resources are not meant to be part of the external api nor part of the bc-promise; the file in question is internal bundle DI configuration and as long the same services parameters are published to DI the existing code will continue to work.

most of the modern bundles nowadays use php configuration

and here's the example of doctrine bundle moving their config without rising the major version (suggesting that no bc has happened) https://github.com/doctrine/DoctrineBundle/commit/fa3e8e30e84727dccc2eb3a2c681246944e433d2#diff-809c32c881217fd455a93ce4f171a881802c6ef64bfe025ed3d38d8d4935d2d2

Regards, k

RanVaknin commented 3 weeks ago

Hey @kaznovac ,

Thanks for the additional info. I see what you are saying regarding the setup. That does make sense. Regarding the PR itself I'll defer this to one of the SDEs to see if the change can be applied.

Thanks, Ran~