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

[Form] Deprecate not configuring the default_protocol option of the UrlType #50922

Merged
merged 1 commit into from Nov 18, 2023

Conversation

MatTheCat
Copy link
Contributor

@MatTheCat MatTheCat commented Jul 9, 2023

Q A
Branch? 7.1
Bug fix? no
New feature? no
Deprecations? yes
Tickets Part of #50871
License MIT
Doc PR N/A

You would expect an <input type="url"> from the UrlType, but it is not possible as long as default_protocol has a value, because then you have to accept inputs that are not URLs (and you get an <input type="text" inputmode="url">).

In order to change default_protocol from 'http' to null in 8.0, we must first deprecate not configuring it.

@xabbuh
Copy link
Member

xabbuh commented Jul 19, 2023

IIRC we overrode the tests in other cases. But we should add a comment that the overridden methods can be removed in the 7.0 branch.

@MatTheCat
Copy link
Contributor Author

Thanks @xabbuh, hope I did it correctly

@MatTheCat
Copy link
Contributor Author

Is it still time to merge this in 6.4 or will I have to rebase on 7.1 when the branch will be created?

@stof
Copy link
Member

stof commented Nov 7, 2023

we should have a test ensuring that passing null explicitly (to not have any default protocol) works without triggering a deprecation. Otherwise it will make it impossible to have that behavior (which is the wanted one when you want an <input type="url">) without triggering a deprecation.

@MatTheCat
Copy link
Contributor Author

@stof thanks for the review 🙏

Do you know how I can test whether a given code does not trigger a deprecation?

@stof
Copy link
Member

stof commented Nov 7, 2023

@MatTheCat Don't put the @group legacy on the test, and the phpunit bridge will already make the test fail if it triggers a deprecation.
But looking at the test file in full (not visible in the diff of the PR), we already have some tests passing null explicitly so this will be fine.

@nicolas-grekas
Copy link
Member

You should add a test that runs into the situation, and if it triggers a deprecation, the CI will fail.

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

I think this can wait for 7.1

src/Symfony/Component/Form/Extension/Core/Type/UrlType.php Outdated Show resolved Hide resolved
@nicolas-grekas nicolas-grekas modified the milestones: 6.4, 7.1 Nov 15, 2023
@MatTheCat MatTheCat force-pushed the ticket_50871 branch 3 times, most recently from 16bcc43 to 286bab1 Compare November 18, 2023 14:46
@fabpot
Copy link
Member

fabpot commented Nov 18, 2023

Thank you @MatTheCat.

@fabpot fabpot merged commit 32ab86f into symfony:7.1 Nov 18, 2023
3 of 9 checks passed
@MatTheCat MatTheCat deleted the ticket_50871 branch November 18, 2023 20:45
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.

None yet

6 participants