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.

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

Fix warnings deprecation and test errors #40

Merged
merged 1 commit into from
Aug 22, 2019
Merged

Fix warnings deprecation and test errors #40

merged 1 commit into from
Aug 22, 2019

Conversation

alexzerah
Copy link
Contributor

@alexzerah alexzerah commented Aug 21, 2019 •
edited
Loading

Details

  • Fix warnings deprecation

Check

  • Tests are ok 👍
  • CS are ok 👍

@lyrixx
Copy link
Member

lyrixx commented Aug 21, 2019

IMHO, we should add a conflict rule in composer.json to forgot version of symfony that are not compatible anymore. This changes landed in symfony 4.2. So now this bundle is incompatible with symfony 3.4 😬

Another way to deal with it, it to support both methods: https://github.com/symfony/monolog-bundle/blob/635bb23451665247ad676f6b657d3f8c52f94705/DependencyInjection/Configuration.php#L338-L339

@alexzerah alexzerah changed the title Fix warnings deprecation Fix warnings deprecation and test errors Aug 21, 2019
@alexzerah alexzerah mentioned this pull request Aug 21, 2019
Merged
@alexzerah
Copy link
Contributor Author

IMHO, we should add a conflict rule in composer.json to forgot version of symfony that are not compatible anymore. This changes landed in symfony 4.2. So now this bundle is incompatible with symfony 3.4 😬

Another way to deal with it, it to support both methods: https://github.com/symfony/monolog-bundle/blob/635bb23451665247ad676f6b657d3f8c52f94705/DependencyInjection/Configuration.php#L338-L339

Second method seems better for me. Is it better to do that ?

@alexzerah alexzerah force-pushed the fix/warnings-deprecation branch from 97e5358 to b6ad0eb Compare August 21, 2019 12:52
lyrixx
lyrixx reviewed Aug 22, 2019
View reviewed changes
composer.json Outdated Show resolved Hide resolved
@alexzerah alexzerah force-pushed the fix/warnings-deprecation branch from 6fc39a4 to b0c075e Compare August 22, 2019 08:59
@pyrech pyrech force-pushed the fix/warnings-deprecation branch from b0c075e to 9c0c072 Compare August 22, 2019 09:02
@alexzerah alexzerah force-pushed the fix/warnings-deprecation branch 2 times, most recently from 7737e01 to b06d625 Compare August 22, 2019 11:48
pyrech
pyrech previously requested changes Aug 22, 2019
View reviewed changes
Copy link
Member

@pyrech pyrech left a comment

Choose a reason for hiding this comment

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

You forgot to update root_dir usages in the documentation

tests/Functional/Fixtures/symfony/app/config/config.yml Outdated Show resolved Hide resolved
lyrixx
lyrixx reviewed Aug 22, 2019
View reviewed changes
composer.json Outdated Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
tests/Functional/Fixtures/symfony/app/config/config.yml Show resolved Hide resolved
@alexzerah alexzerah force-pushed the fix/warnings-deprecation branch from 92f19c5 to 9c24593 Compare August 22, 2019 13:39
@alexzerah alexzerah force-pushed the fix/warnings-deprecation branch from 9c24593 to d0681ea Compare August 22, 2019 13:52
@lyrixx lyrixx dismissed pyrech’s stale review August 22, 2019 14:04

this is ok now

@pyrech pyrech merged commit a1ac39a into master Aug 22, 2019
@pyrech pyrech deleted the fix/warnings-deprecation branch August 22, 2019 14:04
welcoMattic pushed a commit that referenced this pull request May 6, 2020
@alexzerah @welcoMattic
Fix deprecation messages and fix lowest package dependencies (#40)
3228129 
Thanks!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lyrixx lyrixx lyrixx approved these changes

@damienalexandre damienalexandre damienalexandre approved these changes

@pyrech pyrech pyrech left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@alexzerah @lyrixx @damienalexandre @pyrech