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

[5.7] Further renaming of scheme from schema #26640

Merged
merged 1 commit into from Nov 28, 2018

Conversation

Projects
None yet
3 participants
@ralphschindler
Contributor

ralphschindler commented Nov 27, 2018

Further fix naming of scheme for schema as parameter in forceScheme().

A separate PR for 5.8 will follow that will further rename the protected property $cachedSchema to $cachedScheme. That change is left out of 5.7 since it is a minor BC break. In 5.8, even though it is a minor BC break, the likelihood of anyone using $cachedSchema is extremely low.

No tests here as there is not testable changes (in behavior).

*
* @deprecated In 5.8, this will change to $cachedScheme

This comment has been minimized.

@ntzm

ntzm Nov 27, 2018

Contributor

If it's deprecated, you need something to supersede it

This comment has been minimized.

@ralphschindler

ralphschindler Nov 27, 2018

Contributor

If the name were private it would not constitute a minor BC change. I don't actually think people are adding macros to UrlGenerator or extending the class in a way where they are directly accessing the cached/in-memory value of the calculated scheme, but to do our due diligence I think we should push the rename to 5.8, just in case.

In the mean time, there is nothing that can simply supersede the original protected property. All it does is track some state, and introducing a secondary (newly named) property would unnecessarily complicate the code for no real value.

@taylorotwell

This comment has been minimized.

Member

taylorotwell commented Nov 28, 2018

Thanks

@taylorotwell taylorotwell merged commit 2f3f0ff into laravel:5.7 Nov 28, 2018

2 checks passed

continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

TBlindaruk added a commit to TBlindaruk/laravel-framework that referenced this pull request Dec 2, 2018

[5.8] Rename `$cachedSchema` property to `$cachedScheme` in `Routing\…
…UrlGenerator`

 - rename `$cachedSchema` property to `$cachedScheme` in `Routing\UrlGenerator`,
 since in laravel#26640 PR property is deprecated;

TBlindaruk added a commit to TBlindaruk/laravel-framework that referenced this pull request Dec 3, 2018

[5.8] Rename `$cachedSchema` property to `$cachedScheme` in `Routing\…
…UrlGenerator`

 - rename `$cachedSchema` property to `$cachedScheme` in `Routing\UrlGenerator`,
 since in laravel#26640 PR property is deprecated;

taylorotwell added a commit that referenced this pull request Dec 4, 2018

[5.8] Rename `$cachedSchema` property to `$cachedScheme` in `Routing\…
…UrlGenerator` (#26728)

- rename `$cachedSchema` property to `$cachedScheme` in `Routing\UrlGenerator`,
 since in #26640 PR property is deprecated;

taylorotwell added a commit to illuminate/routing that referenced this pull request Dec 4, 2018

[5.8] Rename `$cachedSchema` property to `$cachedScheme` in `Routing\…
…UrlGenerator` (#26728)

- rename `$cachedSchema` property to `$cachedScheme` in `Routing\UrlGenerator`,
 since in laravel/framework#26640 PR property is deprecated;
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment