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

[5.8] Deprecate useless Arr and Str global helper methods #26898

Conversation

X-Coder264
Copy link
Contributor

The goal of this PR is to deprecate the global Arr and Str helper methods for Laravel 5.8 and then remove them in Laravel 5.9 as they are totally useless.

They pollute the global namespace and they don't bring any additional value to the framework. They don't even save you the amount of characters that you have to type as in a bunch (or maybe in all?) of cases it's actually shorter to use the Arr and Str methods directly.

@mfn
Copy link
Contributor

mfn commented Dec 18, 2018

This has my univocal support, thanks for coming up with this 👍

@mfn
Copy link
Contributor

mfn commented Dec 18, 2018

Nice reference for others btw. #25421 (comment)

@SjorsO
Copy link
Contributor

SjorsO commented Dec 19, 2018

I've always liked the convenience if being able to use some of these helpers without having to add an import (especially the str_random, ends_with, starts_with, str_after, str_before helpers). But defining these in my own helpers.php is trivial, so i guess i don't mind if they are removed from core.

However, could you explain what you mean by "they pollute the global namespace"? what are concrete issues these global helpers cause?

@deleugpn
Copy link
Contributor

My only problem with this PR is it's negative vibe. "Useless" or "pollution" are a rather matter of opinion.
I don't think Laravel got to where it is now by providing useless features. The helpers are not useless, but they are, indeed, helpers. It's not hard to port them to a package or userland code if one desires.
The global namespace "problem" could be solved by namespacing the helpers, but perhaps Laravel decides to take a different stand this time.
Let's deprecate these nice little helpers that once helped a lot of people in the community, but that no longer align with the framework goals.

@taylorotwell
Copy link
Member

I think deprecating them is a good idea because right now we force them on developers and they do add a lot of functions to the global namespace that can conflict with other packages, etc.

That being said, I think we should package them up in a laravel/helpers Composer package that people can pull into their projects if they want to quickly get them back in.

@taylorotwell taylorotwell merged commit 837e6c5 into laravel:master Dec 19, 2018
@X-Coder264 X-Coder264 deleted the deprecate-useless-arr-and-str-global-helpers branch December 19, 2018 13:55
@driesvints
Copy link
Member

driesvints commented Dec 20, 2018

@taylorotwell Spatie already has a nice array-functions package. Maybe they're willing to add these? https://github.com/spatie/array-functions

/cc @freekmurze

@freekmurze
Copy link
Contributor

We accept PRs with tests and updated docs that add functions to our package for sure.

But in this case Laravel would still keep the implementations itself in the Str and Arr classes. A first party helper package that just proxies function calls to these classes makes more sense than duplicating the implementation to a third party package.

@tontonsb
Copy link
Contributor

tontonsb commented Feb 1, 2019

What about docs?

It seems that currently Str and Arr classes aren't mentioned in the documentation at all. Some of these helpers are very useful and it should be possible for new developers to discover them through docs whatever the syntax is.

And maybe there should be some more clear aliases in the classes themselves? I understand snake_case($s) without knowing it, but it wouldn't be that clear what \Str::snake($s) is doing.

@devcircus
Copy link
Contributor

These big changes are usually meticulously documented. I'm sure this will be added when they start aggregating all of the changes and getting the documentation ready for the release of 5.8.

Also if you have any ideas for documentation improvement, the docs are a community-driven, open source project. So feel free to make them better.

@robclancy
Copy link
Contributor

I prefer the fucntions :( I don't want to bring in a namespace everytime.

@mfn
Copy link
Contributor

mfn commented Feb 1, 2019

Unfortunately they're necessary. Global functions are a cancer and since they're not opt-in at the moment, the framework is best advised to not use them.

I'm sure someone will whip up a package with all the global helper functions desired.

@deleugpn
Copy link
Contributor

deleugpn commented Feb 1, 2019

I think the negativeness here is a lot more toxic than the little helpers. Cancer is a incredibly strong word for functions that have no impact on whoever is not interested in using them.

@devcircus
Copy link
Contributor

I've had issues on several projects due to name collisions. Maybe cancer is a bit strong(as a cancer survivor) but we had gotten to the point where a new new global or Blade global was being added once a week or more. Personally glad they're going away.

@mfn
Copy link
Contributor

mfn commented Feb 1, 2019

I apologize for the wording, I didn't realize it was perceived this way. I lobbied a lot to get rid of them:

functions that have no impact on whoever is not interested in using them

However this I feel reflects an untrue state of affairs, and especially due to their generic names, they may clash with other (older, legacy) projects which I personally have experienced. And since they're currently not opt-in (or: you can't even opt-out), they are a problem.

@baoanhng
Copy link

baoanhng commented Feb 2, 2019

Is it a good idea to declare those helper classes as alias in app config by default?

@ohnotnow
Copy link
Contributor

ohnotnow commented Feb 2, 2019

Is a six-month window a long enough deprecation notice for this? I can't begin to imagine the number of examples on blogs, video tutorials, books etc this is going to break? I work with a lot of legacy code too - and I'm all for things that make it easier to pull in framework components to them - but given the scope and how many non-legacy codebases this is going to touch it seems a little... abrupt?

Would a very strongly worded notice on 5.8 then doubling down coming 5.9 be a little kinder on the wider laravel world?

@devcircus
Copy link
Contributor

devcircus commented Feb 2, 2019

On the other hand, surely we aren't blindly upgrading major versions of Laravel without reading the upgrade guide for breaking changes. There's no automatic upgrade in place that is going to break apps. The developer has all the time needed to make the changes necessary before upgrading. This change will be front and center in the upgrade guide with detailed instructions on what to change.

In addition, if it is put off until 5.9, the vast majority will likely wait until 5.9 is released and find themselves in the same situation. I think it is always better to just pull the trigger on these changes and let everyone take all the time they need to make the changes necessary before upgrading.

Finally, using Laravel Shift by Jack McDade will take care of this very easily.

@ohnotnow
Copy link
Contributor

ohnotnow commented Feb 2, 2019

I'm not especially thinking about upgrading by people who dutifully pay attention to the docs or can use shift - it's more the number of help sites, videos, documentation that are going to be broken. At least a longer deprecation window gives some time for those older blogs, videos etc to fade from the first hits on google etc, video tutorials to be replaced by newer ones and so on. New users who just do a laravel new my-first-project while following along a fairly recent book, article, video or whatever will be hit then start flooding help forums or just giving up.

I just feel like they've been around so long in the framework and are so embedded 'out there' that a very strongly worded message in the upgrade guide this time round - then switching the helpers documentation around to use the Arr/Str classes instead with an explanation would save a lot of pain. Would six months more grace on the change be that bad? I don't think there has been a huge upswelling out there for them to be removed (though I agree they should be!) so I'm not convinced there's a great urgency about it?

It's not a great final hill I'm going to die for or anything - just raising the prospect that it's going to cause quite a lot low-level grumbling, confusion etc - and maybe a longer deprecation window would help :-)

@aurawindsurfing
Copy link

As someone relatively new to the framework I also second @ohnotnow opinion here. I understand the reasoning behind it but in my opinion, it makes Laravel more of a pure framework open for custom configuration like with Symphony then an opinionated @taylorotwell framework. Having said that it is at the end of the day @taylorotwell who has the last say here. Just feels a bit strange not to be able to use anymore something I have used a lot as a beginner.

@martinbean
Copy link
Contributor

So will other helpers like route(), url(), view() etc be getting deprecated in the future, too?

@emredipi
Copy link

emredipi commented Feb 3, 2019

How about making a String class like java, and we call it to use string stuffs. Also this will allow us to use oop structure. For instance; I do not like explode function usage. Php is missing about this.

@unstoppablecarl
Copy link
Contributor

unstoppablecarl commented Feb 3, 2019

Would namespacing the functions solve the global pollution problem? Isn't that what they are for? I have always wondered why this feature appears to never be used in PHP.

@garygreen
Copy link
Contributor

Love this.

Glad to see my suggestion waaay back has finally got traction. laravel/ideas#194

😍

@garygreen
Copy link
Contributor

garygreen commented Feb 8, 2019

So will other helpers like route(), url(), view() etc be getting deprecated in the future, too?

I would say no. I would argue those helpers are commonly used in the majority of applications. But I wouldn't really care if they were removed, too - I would just add any needed ones into my app.

The helpers being removed here are ones that I would suggest are not as widely used, e.g. array_random array_sort_recursive str_after.

Also, their usage is no shorter e.g. str_after vs Str::after, whereas url() is much shorter than URL::to() or view() vs View::make()

Is there the added complexity of having to import the Arr or Str namespace? Sure. But that's something IDE tools can easily solve, and you can create your own aliases if it's too much trouble.

I think there should be a general rule drafted where if a helper is expected to be used in >= 80% of applications AND it's shorter to write, then the helper stays, if not, then it goes, IMO.

@robclancy
Copy link
Contributor

Is a six-month window a long enough deprecation notice for this? I can't begin to imagine the number of examples on blogs, video tutorials, books etc this is going to break? I work with a lot of legacy code too - and I'm all for things that make it easier to pull in framework components to them - but given the scope and how many non-legacy codebases this is going to touch it seems a little... abrupt?

Would a very strongly worded notice on 5.8 then doubling down coming 5.9 be a little kinder on the wider laravel world?

Problem is PHP (or Laravel) doesn't give notice about depredations except for a single place in the docs. JS frameworks can generally deprecate nicely because they can console.log. Now we have Laravel Telescope I think it should be updated to have a "deprecations" tab. Then in Laravel have a deprecation method that will log to Telescope, log files, or whatever. Then I could get behind a 6 month deprecation.

Here is an example in ember. https://github.com/emberjs/ember.js/blob/c5a531e64904ade433a67c0cbbd779759de1df09/packages/ember/index.js#L154
It gives a message on how to fix, a link to the message in more detail, and a version where the functionality is removed.

This has the added benefit of spamming people when deprecations are in addons so people will complain to addon users or PR to the addon and you don't see them being outdated. Whereas with Laravel a deprecation for something a library uses is unlikely to be fixed until the deprecation period is over and they get errors.

But without that, this deprecation should be at least a year. 2 years should be standard imo.

@devcircus
Copy link
Contributor

Based on recent commits, it looks like laravel is taking strides to better handle depreciations.

@deleugpn
Copy link
Contributor

deleugpn commented Feb 8, 2019

Given the fact that there will be a package that can be installed and keep the entire application working as-is, I think this deprecation doesn't require more than 6 months. It could easily be removed on 5.8 as well as it's just a matter of when the developer wants to drop (or not) the optional package.

@robclancy
Copy link
Contributor

No deprecation should ever be as short as 6 months.

@deleugpn
Copy link
Contributor

deleugpn commented Feb 8, 2019

Let's agree to disagree.

@garygreen
Copy link
Contributor

garygreen commented Feb 8, 2019

I disagree too. Remember - breaking changes between patch versions of Laravel is to be expected, and this one is MINOR given the fact that you can drop-in a package that adds these helpers back in. It should then be encouraged for people to slowly migrate away from the helpers, or create their own aliases (IMO)

Who's going to maintain that separate package, well I don't know as that's the whole benefit of this PR - it's to avoid maintaining pointless aliases/shortcuts with mirror parameter signatures, docblocks, etc when they can be easily replaced with a direct call.

@garygreen
Copy link
Contributor

garygreen commented Feb 8, 2019

Would it be an idea to deprecate functions by triggering a notice? That way developers are more likely to take action, as most won't bother looking at Laravel internal code.

function array_add($array, $key, $value)
{
    trigger_error('Method ' . __METHOD__ . ' is deprecated', E_USER_DEPRECATED);
    // ....
}

This also fits in with the workflow of when PHP functions are deprecated

@devcircus
Copy link
Contributor

I've missed the deprecation rulebook that must be floating around. Upgrading a major version of a web framework without consulting the docs and/or code itself, is a disaster waiting to happen.

@X-Coder264
Copy link
Contributor Author

X-Coder264 commented Feb 8, 2019

Yes, it would be really cool for deprecations to be also triggered in the code so that we can create a PR for Telescope so that we can show them there in a special tab, just like the Symfony toolbar does.

As for the above discussion about the deprecation period, 6 months is more than enough, especially for such a change which is gonna take a minimal effort (just requiring the new package) in order to be "solved". If it were up to me this PR would've actually deleted them completely from the framework for 5.8, but I knew Taylor wouldn't merge such a PR which is why I sent this one instead 😄

Somebody mentioned above that the standard deprecation period should be 2 years, I think that's crazy. If we'd go for a 2 year deprecation period as a standard then what is the point of Laravel getting a "major" version (which allows breaking changes) every six months? For that to make sense we'd have to use semantic versioning for Laravel and release "major" versions only once every 2 years which would only slow down the development of the framework as we'd have a bunch of "deprecated" code to maintain as well, just like Symfony does now. I know that a lot of people (especially the "enterprise" ones) out there prefer Symfony over Laravel for that exact reason, but I personally see that as a pro for Laravel instead of a con.

@robclancy
Copy link
Contributor

I've missed the deprecation rulebook that must be floating around. Upgrading a major version of a web framework without consulting the docs and/or code itself, is a disaster waiting to happen.

Are the docs going to magically go over all your libraries for you and make a PR to each of them where there is a change?

@X-Coder264
Copy link
Contributor Author

X-Coder264 commented Feb 8, 2019

Having a 6 month deprecation period means you have half a year to solve the deprecation, that's more than enough in my book, especially for such minor changes which you can solve by using an IDE and do a find and replace on the codebase and be done with it in a couple of minutes. And that's only if you want to update to the new major version on the same day that it's released which nobody is forcing you to do. And 5.7 doesn't become unusable or something just because 5.8 gets released. Upgrade 6 months (or even more) after the major version gets released. Then you will have 1 year+ to solve the deprecations.

@robclancy
Copy link
Contributor

Libraries aren't going to updated and like everytime laravel has a release and people will be waiting on authors all over the place to fix the completely broken package because there is no real deprecation system in place. Something the author had no idea was going to be broken because they hadn't yet updated their own app and seen the deprecation note (even then they would probably still miss it... because it's a small note on a doc site lol).

@X-Coder264
Copy link
Contributor Author

X-Coder264 commented Feb 8, 2019

There isn't gonna be a "broken" package because the package should add to its composer.json that it allows 5.8 only when they fix the deprecations. If their package composer constraint is wrong (and it allows you for example to install the package with any Laravel 5.X version) then that is the package's fault.

(even then they would probably still miss it... because it's a small note on a doc site lol).

Sorry, but this is so funny, the note is anything but small. And not only is the note quite big in the official upgrade guide, but it's also reported on sites like laravel-news etc. so it's basically next to impossible to miss unless you are trying very hard to miss it 😄

@robclancy
Copy link
Contributor

How can you miss the point so badly? when they fix the deprecations lol that's not how it works when someone doesn't fix a deprecation lmao.
Read the first 5 words.

@X-Coder264
Copy link
Contributor Author

You said there are gonna be broken packages. I said there won't be any unless the package composer constraint is wrong which is not a Laravel framework issue, but an issue with the package.

Besides, I don't know what "obscure" packages are you using, but all important and widely used Laravel packages (like for example the Spatie ones) get updates on like day one.

@robclancy
Copy link
Contributor

"I don't know what packages are going to broken except for packages with issues" - you lmao

TIL people only use packages you use. Mr X-Coder264.

Never in the history of packages (except for all those times you see PRs on packages every release and then people complaining it isn't merged) has there been someone who hasn't updated for a breaking change because of the lack of a deprecation system like you see in other frameworks.

Pack it up folks. Restrict yourself to doing basic projects and using Spatie packages instead of improving Laravel.

@laravel laravel locked as too heated and limited conversation to collaborators Feb 8, 2019
@driesvints
Copy link
Member

I'm going to lock this because it seems that this has gone off topic quite a bit...

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.