Skip to content

[Notifier] Improve tests (5.1) #39509

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
Dec 16, 2020

Conversation

OskarStark
Copy link
Contributor

@OskarStark OskarStark commented Dec 15, 2020

Q A
Branch? 5.1
Bug fix? no
New feature? no
Deprecations? no
Tickets ---
License MIT
Doc PR ---

Please review commit by commit

@carsonbot carsonbot added this to the 5.1 milestone Dec 15, 2020
@OskarStark OskarStark force-pushed the notifier-5.1-test-improvements branch 3 times, most recently from 711aa79 to bd3513c Compare December 15, 2020 10:25
fabpot added a commit that referenced this pull request Dec 15, 2020
… (OskarStark)

This PR was merged into the 5.1 branch.

Discussion
----------

[Notifier]  [Free Mobile] Could not use custom host in DSN

| Q             | A
| ------------- | ---
| Branch?       | 5.1
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | ---
| License       | MIT
| Doc PR        | ---

While working on #39509 I discovered, that you cannot set a custom host through the DSN string itself, only by calling `setHost()` method in the transport, which is only possible by **not** using the factory....

I changed it the way all other bridges work. I don't add a testcase for the port, because non of the others have that test.
I plan to implement it in #39495

As this is a bugfix I created an extra PR.

Cheers

EDIT:

Also the host is not allowed to contain `https://` otherwise calling `__toString()` will result in: `freemobile://https://......`

Commits
-------

63350cc [Notifier] [Free Mobile] Could not use custom host in DSN
@OskarStark OskarStark force-pushed the notifier-5.1-test-improvements branch from bd3513c to 9cabbcb Compare December 15, 2020 18:53
@OskarStark
Copy link
Contributor Author

Ready to merge from my side, if tests pass 😄

Good night! 🌔

@derrabus derrabus force-pushed the notifier-5.1-test-improvements branch from 9cabbcb to 5773a46 Compare December 16, 2020 07:45
@derrabus
Copy link
Member

Thank you Oskar.

@derrabus derrabus merged commit 40672e1 into symfony:5.1 Dec 16, 2020
@OskarStark OskarStark deleted the notifier-5.1-test-improvements branch December 16, 2020 08:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants