Skip to content

fix: check if session if available #478

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

Merged
merged 1 commit into from
Sep 2, 2022

Conversation

robbedg
Copy link
Contributor

@robbedg robbedg commented Sep 1, 2022

In some requests no session is available.
I have this problem specifically when using cross domain requests in the preflight request.
This gives an error in the activator for 'edit in place'. Checking if the session is available before executing the code solves the problem.

Copy link
Member

@welcoMattic welcoMattic left a comment

Choose a reason for hiding this comment

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

LGTM, CI fails are due to deprecation messages. Should we ignore them in the PR? cc @php-translation/core

@rvanlaak
Copy link
Member

rvanlaak commented Sep 1, 2022

@welcoMattic how much time would it take us to fix these deprecations, would we be able to estimate that?

Preferably we fix those deprecations on master first, and thereafter rebase this PR, right?

Copy link
Member

@bocharsky-bw bocharsky-bw left a comment

Choose a reason for hiding this comment

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

Looks good to me too, makes sense 👍

What about failed tests: if they're failing only due to deprecations - I'm fine to merge it now and try to fix deprecations in different PRs. Otherwise, we need to fix tests here

@welcoMattic
Copy link
Member

@welcoMattic how much time would it take us to fix these deprecations, would we be able to estimate that?

Preferably we fix those deprecations on master first, and thereafter rebase this PR, right?

It's mostly about Symfony upgrade. I guess no hard work here. I'm fine to merge this PR and fix the deprecations in another one too.

@bocharsky-bw bocharsky-bw merged commit 692900e into php-translation:master Sep 2, 2022
@bocharsky-bw
Copy link
Member

OK, great! Thank you @robbedg for this fix! Would you mind helping with fixing failing tests too?

@robbedg
Copy link
Contributor Author

robbedg commented Sep 5, 2022

@bocharsky-bw Thanks for the merge. Unfortunately I'm not that familiar with the tests. I have seen you have made a new PR. If I find anything that can help I'll put it there.

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.

4 participants