-
Notifications
You must be signed in to change notification settings - Fork 11.2k
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
[5.8] Deprecate useless Arr and Str global helper methods #26898
Conversation
This has my univocal support, thanks for coming up with this 👍 |
Nice reference for others btw. #25421 (comment) |
I've always liked the convenience if being able to use some of these helpers without having to add an import (especially the However, could you explain what you mean by "they pollute the global namespace"? what are concrete issues these global helpers cause? |
My only problem with this PR is it's negative vibe. "Useless" or "pollution" are a rather matter of opinion. |
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 |
@taylorotwell Spatie already has a nice /cc @freekmurze |
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 |
What about docs? It seems that currently And maybe there should be some more clear aliases in the classes themselves? I understand |
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. |
I prefer the fucntions :( I don't want to bring in a namespace everytime. |
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. |
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. |
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. |
I apologize for the wording, I didn't realize it was perceived this way. I lobbied a lot to get rid of 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. |
Is it a good idea to declare those helper classes as alias in app config by default? |
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? |
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. |
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 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 :-) |
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. |
So will other helpers like |
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. |
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. |
Love this. Glad to see my suggestion waaay back has finally got traction. laravel/ideas#194 😍 |
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. Also, their usage is no shorter e.g. Is there the added complexity of having to import the 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. |
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 Here is an example in ember. https://github.com/emberjs/ember.js/blob/c5a531e64904ade433a67c0cbbd779759de1df09/packages/ember/index.js#L154 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. |
Based on recent commits, it looks like laravel is taking strides to better handle depreciations. |
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. |
No deprecation should ever be as short as 6 months. |
Let's agree to disagree. |
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. |
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 |
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. |
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. |
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? |
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. |
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). |
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.
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 😄 |
How can you miss the point so badly? |
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. |
"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. |
I'm going to lock this because it seems that this has gone off topic quite a bit... |
The goal of this PR is to deprecate the global
Arr
andStr
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
andStr
methods directly.