search

kohana / core

Core system classes from Kohana
http://kohanaframework.org
636 stars 327 forks source link

Deprecate View::set_global #587

Closed cyrrill closed 9 years ago

cyrrill commented 9 years ago

In efforts to improve the architecture of Kohana, and move away from static global scope, I propose that we deprecate View::set_global and associated functionality in version 3.4. PR upcoming if there is consensus.

acoulton commented 9 years ago

:+1:

rjd22 commented 9 years ago

:+1:

enov commented 9 years ago

Removing View::set_global will not directly improve the architecture of Kohana, per se. As it is not used within the core, nor it is used anywhere else in the official modules.

But, it might improve the architecture of the applications relying on that as a convenience method; as well as the code of third party modules, in case they add variables through that and pollute the View's global scope. It seems deprecating is just a warning towards best practices, and removing it is only improving the applications made on top of Kohana.

I still can not decide whether to say yes or no here, as it does not seem an urgent matter that will fix Kohana itself. We are just removing a bad choice from the developers' hand.

enov commented 9 years ago

And there is also View::bind_global to take into consideration.

enov commented 9 years ago

@cyrgit frankly, I don't understand your tone and why you have closed the issue.

I was trying to make a point here that this won't affect Kohana's architecture as it is not used anyware. If we're going to remove View::set_global and state that we have improved architecture, it's lying.

I also did not defend its usage. I have properly marked as a bad choice for developers.

I did not see this as urgent in terms of removing them is breaking compatibility, besides removing a choice.

@enov, enjoy using your global statics I won't be here to remove them for you! :)

This is childish @cyrgit and you're the last person I would expect such a comment. It's not that every time there are words like "global" and "static" that it is forbidden to challenge and discuss.

lenton commented 9 years ago

:+1:

enov commented 9 years ago

I am really sorry if somewhere I hurt you @cyrgit.

acoulton commented 9 years ago

@cyrgit sorry if you're offended but I think you're overreacting. You asked for discussion/consensus, you got some +1s and then @enov said essentially "meh". He didn't defend the statics, just noted he didn't really think it was urgent.

As you know, every commit takes time and every open issue creates noise. The old issue tracker is full of "yeah, ok, not urgent" stuff that nobody has taken on. @enov has spent a lot of time lately just to isolate the actual live 3.3 issues from the cruft. We're a small community developer team and you're not the only one with a real job. I read @enov's comment as yeah, ok, don't feel I need to carve out time to do this but sure. 2 :+1:s and a "meh" is not getting stuck...

If you'd sent a PR with the @deprecated tags and updated documentation I'd have just merged it and that would have been that. See #588

Sure, we're not taking every change - especially those that seem a lot of work for small benefit, or particularly ones that break BC for all the many people with real jobs trying their best to keep applications going. And we're moving slowly. But there are a bunch of positive changes coming through too, not everything gets stuck.

enov commented 9 years ago

Thanks @acoulton.

zombor commented 9 years ago

I've been trying to get rid of set_global for 6 years.

rjd22 commented 9 years ago

@cyrgit I really hope you will continue to work with us. Keep in mind that we're all humans and make mistakes and are doing the best we can.