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

Add compatibility to Microsoft Edge #60

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

zxy19
Copy link

@zxy19 zxy19 commented Jun 24, 2024

Default string field with length 255 is too short for Microsoft Edge, which uses endpoint url up to 500 characters long.
Change length of field endpoint in table push_subscriptions to 1024.

…patible with Edge (It's endpoint url is up to 500 characters long).
@DellZHackintosh
Copy link
Contributor

I'm so glad to see that - I have distressed on Microsoft Edge that can never receive the notifications for a long time. 👍
Where did you find the documents for this?

@zxy19
Copy link
Author

zxy19 commented Jun 28, 2024

I tried to look up the description of the URL on Microsoft's website, but I got nothing but a statement that "URLs should be treated as black boxes"😟. So I checked the URLs posted in Devtool and compared them with the database, and finally found the problem.
I thought it would be a good idea to reserve some space in order to avoid problems once other browsers use longer URLs (since endpoint URLs don't seem to be specified), so I set it to 1024 characters.

In current version it can be seen that the Endpoint URL stored in database is much shorter than in the post request the browser had sent.So that once Flarum tries to push notice then the webpush library will receive a 404 error and treat the URL as outdated.

@DellZHackintosh
Copy link
Contributor

DellZHackintosh commented Jun 28, 2024

That's funny, when the debug mode is enabled, it told me that notifications have sent to my Edge🤣🤣🤣, maybe we had never consider this situation.
Anyway, thanks for your finding. 👍I'm excited to be able to receive notifications soon. 😊

By the way, this PR seems only changed the tables that from previous version - is that necessary to do more work to create a table with correct length for the new installations?

@askvortsov1
Copy link
Owner

By the way, this PR seems only changed the tables that from previous version - is that necessary to do more work to create a table with correct length for the new installations?

All migrations run on new installs, so this should work fine once it's merged.

@DellZHackintosh
Copy link
Contributor

Has this PR tested on MSEdge?

@zxy19
Copy link
Author

zxy19 commented Jul 4, 2024

Has this PR tested on MSEdge?

Yes, with verion 126.0.2592.81 on my computer.

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.

3 participants