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

chore: support symfony 5 #75

Merged
merged 10 commits into from
Jul 26, 2020
Merged

chore: support symfony 5 #75

merged 10 commits into from
Jul 26, 2020

Conversation

SelviA
Copy link
Contributor

@SelviA SelviA commented Jul 14, 2020

  • DataCollector::collect method signature change since SF 4.4
// before Symfony 4.4
public function collect(Request $request, Response $response, \Exception $exception = null);

// since Symfony 4.4
public function collect(Request $request, Response $response, \Throwable $exception = null);

@see symfony/symfony@ffcfdb4

  • Renamed events :
    GetResponseForExceptionEvent replaced by ExceptionEvent
    PostResponseEvent replaced by TerminateEvent

  • Swap arguments of dispatch()
    @see symfony/symfony@75369da

  • Use the spaceless filter instead of spaceless tag (since Twig 1.38)

DataCollector::collect method signature change since SF 4.4
Copy link
Member

@Oliboy50 Oliboy50 left a comment

Choose a reason for hiding this comment

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

Thanks 👌

I just wonder why are you dropping older symfony versions support in this PR?

using \Throwable instead of \Exception is not a breaking change for me since Throwable is wider than Exception 🤔

Selvi ARIK added 4 commits July 15, 2020 09:28
@SelviA SelviA requested a review from Oliboy50 July 15, 2020 13:45
@SelviA
Copy link
Contributor Author

SelviA commented Jul 16, 2020

Hi @Oliboy50,

IMO it's a little bit difficult to keep support of Symfony versions before 4.4 but let's try :

  • The signature of the DataCollector::collect method has been updated since 4.4 and unless I am mistaken PHP only supports covariance since 7.4 version (which is supports since 4.4 version of Symfony)
  • The signature of the EventDispatcherInterface::dispatch() method has been updated. Maybe we can use the LegacyEventDispatcherProxy decorator as suggests here => I can do it if you want 👍
  • For the renamed events GetResponseForExceptionEvent and PostResponseEvent, no idea to keep compatibility.

I think it's better to allow recent applications to use this bundle but I am listening to you if you have any suggestions.

Copy link
Member

@Oliboy50 Oliboy50 left a comment

Choose a reason for hiding this comment

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

Indeed, full covariance started at php7.4, so you're right, we won't be able to keep symfony <4.4 support here

src/Resources/views/Collector/statsd.html.twig Outdated Show resolved Hide resolved
src/Resources/views/Collector/statsd.html.twig Outdated Show resolved Hide resolved
Copy link
Member

@Oliboy50 Oliboy50 left a comment

Choose a reason for hiding this comment

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

👌

@SelviA
Copy link
Contributor Author

SelviA commented Jul 22, 2020

@Oliboy50 Ready for a new release ?

.travis.yml Outdated
jobs:
exclude:
- php: 7.1
env: SYMFONY_VERSION=5.*.*

Choose a reason for hiding this comment

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

Could you please add a new line at the end of this file 🙇

@SelviA
Copy link
Contributor Author

SelviA commented Jul 23, 2020

Are we ready for a new release ?

@Oliboy50 Oliboy50 merged commit 319f7fc into BedrockStreaming:master Jul 26, 2020
@Oliboy50
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants