Skip to content
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.

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

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

Open
kaznovac opened this issue Mar 2, 2023 · 3 comments · May be fixed by #99
Open

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

kaznovac opened this issue Mar 2, 2023 · 3 comments · May be fixed by #99
Assignees
Labels
bug This issue is a bug. p2 This is a standard priority issue queued This issues is on the AWS team's backlog

Comments

@kaznovac
Copy link

kaznovac commented Mar 2, 2023

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

  • state that the yaml package is required composer
  • use php configuration in the bundle (preferred way)

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

@kaznovac kaznovac added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Mar 2, 2023
@yenfryherrerafeliz yenfryherrerafeliz added the p2 This is a standard priority issue label Apr 7, 2023
@yenfryherrerafeliz yenfryherrerafeliz added investigating This issue is being investigated and/or work is in progress to resolve the issue. and removed needs-triage This issue or PR still needs to be triaged. labels May 15, 2023
@RanVaknin RanVaknin self-assigned this Aug 12, 2024
@RanVaknin
Copy link

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~

@RanVaknin RanVaknin added response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. and removed investigating This issue is being investigated and/or work is in progress to resolve the issue. labels Aug 15, 2024
@kaznovac
Copy link
Author

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) doctrine/DoctrineBundle@fa3e8e3#diff-809c32c881217fd455a93ce4f171a881802c6ef64bfe025ed3d38d8d4935d2d2

Regards,
k

@RanVaknin RanVaknin removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Aug 19, 2024
@RanVaknin
Copy link

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~

@RanVaknin RanVaknin added the queued This issues is on the AWS team's backlog label Aug 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. p2 This is a standard priority issue queued This issues is on the AWS team's backlog
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants