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

Php 8.0 support #23

Merged
merged 12 commits into from
Jan 28, 2021
Merged

Php 8.0 support #23

merged 12 commits into from
Jan 28, 2021

Conversation

hugochinchilla
Copy link
Contributor

@hugochinchilla hugochinchilla commented Jan 21, 2021

Hi, I started working on having php 8.0 compatibility:

  • replaced dflydev/fig-cookies with yiisoft/cookies
  • dropped php7.3 because of yiisoft/cookies
  • updated dflydev/fig-cookies to 3.0
  • depend on doctrine/mongodb-odm instead of doctrine/mongodb-odm-bundle
  • fix doctrine/mongodb-odm on a specific commit in master as there is no release with php8.0 support
  • sentry/sentry-symfony updated to 4.0, which causes sentry to update to 3.x, some changes to SentryMiddleware where needed

I have tested everything with php 7.4 and 8.0

I think to release this a major version change would be required because of the changes in Sentry and dropping php 7.3

@hugochinchilla
Copy link
Contributor Author

dflydev/fig-cookies released today version 3.0 with php 8.0 support, I have reverted the changes to stop using that library, also I have brought back support for php 7.3.

Still a major version should be considered because of the implications of updating sentry to 3.x

@hugochinchilla
Copy link
Contributor Author

As per doctrine/mongodb-odm#2273 we can expect a doctrine/mongodb-odm release today or tomorrow

@Baldinof
Copy link
Owner

Hi, thanks a lot for your work :)

I will try to find some time to review it by the end of this weekend!

@Baldinof
Copy link
Owner

When I wrote the Sentry Middleware, it was not possible to passes an arbitratry PSR request to the Sentry integration, it was always created from superglobals.

Now it seems it can be configured with a RequestFetcher, it's implemented in the bundle here : https://github.com/getsentry/sentry-symfony/blob/master/src/Integration/RequestFetcher.php

With RoadRunner we already have a PSR request, it's a waste of time to recreate one from the Symfony one.

What do you think if we:

  • add a RequestFetcher that decorate the one from Sentry Bundle (the decorator should run the default fetcher if the current runtime is not RoadRunner, see StreamedResponseListener)
  • rewrite the Sentry Middleware to simply configure the PSR request on the decorated RequestFetcher before handling the request (and flush the client)

I don't know if this should be a major release or if a conflict section in composer.json can be enough.

@hugochinchilla
Copy link
Contributor Author

I think this bundle should remain independent from sentry bundle, not everyone using this bundle will use the sentry bundle, they will have it installed because it is a dependendy but this doesn't mean they will have it enabled (in the same way I have doctrine/mongodb-odm-bundle installed but not enabled).

So I think it's better to have independende and recreathe the PSR7 request, it's not that much time, completly neglibible if compared with the real causes of latency in any real web-app, like database queries or any other IO operation. And has the benefit to keeping all core features local to this repository which is easier to test and easier to understand for new people wanting to contribute.

A conflict in composer.json would be enought, I didn't think of that.

@Baldinof
Copy link
Owner

I was not proposing to make sentry mandatory here, just clearing a big part of the SentryMiddleware because it seems that we can rely on the code on their side.

Howeover it's not a big deal, for the php 8 support I can merge this with a conflict section in composer.json.

composer.json Outdated Show resolved Hide resolved
src/EventListener/SentryListener.php Outdated Show resolved Hide resolved
src/Helpers/SentryHelper.php Outdated Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
@Baldinof Baldinof merged commit de8357b into Baldinof:master Jan 28, 2021
@Baldinof
Copy link
Owner

Thank you @hugochinchilla :)

@hugochinchilla
Copy link
Contributor Author

Thanks! Can we have a new release soon?

@Baldinof
Copy link
Owner

I just released 1.4.0.

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.

2 participants