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

Proposal for request global removal #761

Open
foxx opened this issue Jun 5, 2015 · 12 comments
Open

Proposal for request global removal #761

foxx opened this issue Jun 5, 2015 · 12 comments
Labels
Feature This is a feature request, not a bug. Open for discussion. Needs docs This feature lacks documentation Needs input This issue lacks information or cannot be reproduced. Please provide more info. Needs tests This feature or fix needs a test-case to ensure it continues to work in the future.
Milestone

Comments

@foxx
Copy link
Contributor

foxx commented Jun 5, 2015

Currently the request context is stored in the request global, which can encourage anti patterns such as accessing the request object from a database model.

I'm thinking that a cleaner approach would be to pass in the request context to the view function itself, rather than having to access it from a global. This object would then be passed around as/where it was needed, encouraging modularity by design.

def some_view(request):
    print request

Could a core maintainer give their thoughts?

@defnull
Copy link
Member

defnull commented Jun 5, 2015

Yes, you are absolutely right. Problem is: Changing this aspect would break any application ever. The only feasible path I can think of:

0.14. Add a plugin to the core that implements this (perhaps based on bottle-inject?).
0.15. Make the plugin the default and allow the user to explicitly disable it.
0.16. Make the plugin core functionality and instead add a plugin that mimics the old behavior.

The debian guys will probably patch the default to stay the same though, because Bottle 0.x is already used a lot in production. A real API change can only be done with a new major release, but as long as there is a migration path, things should be feasible.

If (and I say if) we want to change this core aspect of the framework, we should do it before 1.0. Would be a nice variation from the almost identically (since copied) Flask API, too.

@defnull defnull added Feature This is a feature request, not a bug. Open for discussion. Undecided Needs input This issue lacks information or cannot be reproduced. Please provide more info. labels Jun 5, 2015
@foxx
Copy link
Contributor Author

foxx commented Jun 5, 2015

LGTM, there certainly needs to be a clear migration path, and plugins seem like the best fit. If this gets accepted then I'll happily contribute some time for a PR.

@camconn
Copy link

camconn commented Jun 13, 2015

Personally, I like the way django does it. The idea of using an internal plugin is quite nice and should be trivial to implement.

@my_app.route('/')
def home_page(request):
    return 'hello, world!'

The syntax seems quite nice, and already fits in with the existing plugin architecture. I'll see if I can write up a simple PR.

I propose 0.13 has a way to expose the new functionality (albeit disabled by default) and 0.14 enables the the plugin functionality by default. There can even be ways to keep the old API around if users really need it.

Moreover, we'll need to include disclaimers in the docs about the changes. This API change will undoubtedly cause confusion to newcomers.

@defnull
Copy link
Member

defnull commented Jun 14, 2015

Implementing such a feature (again, the bottle-inject plugin already is capable of doing this) should be the easy part. The documentation part is probably the hardest one. I suggest we start a new branch for this.

@defnull defnull added this to the Release 0.13 milestone Jun 14, 2015
@defnull defnull added Needs tests This feature or fix needs a test-case to ensure it continues to work in the future. Needs docs This feature lacks documentation and removed Undecided labels Jun 14, 2015
@eric-wieser
Copy link
Contributor

Not a huge fan of this change. request is a nice way to pass session state (ie current user) into templates, without having to forward it at every call to include.

@foxx
Copy link
Contributor Author

foxx commented Jul 6, 2015

@eric-wieser I'm not sure I understand what your issue is. You'd just pass in the request object as normal into your templating library. Unless you're saying that you prefer your templating library to magically detect request from globals, rather than having to pass it in on render(), which is not a recommended approach and is precisely what this proposed change is trying to prevent. If I have misunderstood please let me know

@eric-wieser
Copy link
Contributor

I'm trying to cover this situation, using STPL:

main.tpl

include('components/user-image.tpl')

components/user-image

% if request.user:
    <img src="...">
% else:
    <p>For privacy reasons, images are not shown for anonymous users</p>
% end

I don't want to have to forward the request argument at every step

@defnull
Copy link
Member

defnull commented Aug 17, 2015

I changed the default a while ago to automatically forward all local variables to the included template. So, passing request or is_user=? to main.tpl should be enough.

bottle/bottle.py

Line 3524 in 727ce6d

env = _env.copy()

@foxx
Copy link
Contributor Author

foxx commented Aug 17, 2015

@defnull Are you still happy to have this included in 0.13? Any change of plan (re. plugins) from the previous comments?

@foxx
Copy link
Contributor Author

foxx commented Jan 22, 2016

@defnull Any thoughts on whether this will make it into the core?

@agalera
Copy link
Contributor

agalera commented May 17, 2017

If you implement this feature, could you solve this other question?
#906

@rudolphfroger
Copy link

If you do this for request then please also do this for response.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature This is a feature request, not a bug. Open for discussion. Needs docs This feature lacks documentation Needs input This issue lacks information or cannot be reproduced. Please provide more info. Needs tests This feature or fix needs a test-case to ensure it continues to work in the future.
Projects
None yet
Development

No branches or pull requests

6 participants