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

Add use_appname parameter to the Form, Grid, and Auth classes #911

Closed
wants to merge 4 commits into from

Conversation

kszys
Copy link
Contributor

@kszys kszys commented Aug 19, 2024

Similarly to issues ancountered with URL generation including the appname as discussed in #910 , there is a related issue in Grid class. Grid generates several internal links and until now there are no options to avoid having the appname in those URLs.

The PR propose here adds a parameter use_appname to the Grid signature (sorry Massimo!) and then modifies the internal logic inside the Grid class to use it, when generating internal links. This allows to avoid putting the appname in all the URLs, if not desired.

The use case is well described in #910 , so I will not repeat it here.

I tested the changes, and everything seems to be working as expected, but of course a second look would be very useful. Also, possibly the same functionality may be achieved in a different way.

@mdipierro
Copy link
Contributor

I understand your solution. I want to run more tests before I can be sure this is the right approach. I have limited computer access in the next few days but will try do it tomorrow.

@kszys
Copy link
Contributor Author

kszys commented Aug 19, 2024

Thanks for the feedback! Take your time - I do not want to introduce any new issues ;) Also - if you have a better/more holistic approach to handling routing between custom domains and py4web apps - I am definitely open for discussion/help. I think this is an important feature for production sites. The routes.py in web2py was very useful.

@mdipierro
Copy link
Contributor

mdipierro commented Aug 20, 2024 via email

@kszys
Copy link
Contributor Author

kszys commented Aug 26, 2024

FYI - I am looking now at some issues with Form, and I think there is the same problem with submiting Forms - the app_name is always included in the link. I think this would need to be addressed the same way as in Grid (or in another systematic way). I will try to update this PR to address the Form as well.

@mdipierro
Copy link
Contributor

mdipierro commented Aug 26, 2024 via email

@kszys
Copy link
Contributor Author

kszys commented Aug 26, 2024

I just added the commit to the form.py to fix the issue for the submit buttons in forms. It works for me, but...

In Form and Grid there is a lot of manipulation of urls without actually using the URL helper. I presume it could be fixed in URL helper, but then a lot of changes are needed at least in Form and Grid (and possibly in Auth, as described in #910). 🤷

@kszys
Copy link
Contributor Author

kszys commented Aug 27, 2024

Another update 🤷 The auto-generated Forms when using Grid were not following the same logic - ensured now the use_appname parameter is passed from the Grid to the auto-generated Forms.

… when consturcint "next" links, building forms and when redirecting to "index"
@kszys
Copy link
Contributor Author

kszys commented Aug 29, 2024

So... After the fixes of the Form, I realised I can now fully fix the Auth as well. So I patched it and added to this PR. I believe it is now solving the issue identified in #910 entirely. This PR now appears to comprehensively address this issue across Form, Grid, and Auth utilities. Possibly it requires more testing, as I might not be aware of all the dependencies and possible use cases.

Massimo - I would appreciate you thoughts on this PR. I think the problem described in #910 and affecting Form, Grid, and Auth is something that is important to enable the use of py4web in production environments. It seems like a likely scenerio, where multiple sites would be served from a single py4web installation.

The only alternative that I can think of for such a use case is running a separate py4web process for each site as the _default app. In a way it would make it cleaner - not requiring for instance URL(... use_appname=False) in every single use of the URL helper, but feels like it a bit wasteful and defeats the purpose of py4web supporting multiple applications. This is doable, I guess, but then ideally the documentation should make it clear how such a use case could be handled in production.

@kszys kszys changed the title Add use_appname parameter to the Grid class Add use_appname parameter to the Form, Grid, and Auth classes Aug 29, 2024
@mdipierro
Copy link
Contributor

I think the proper way to fix this is to use URL everywhere and add some smartness in URL as opposed to pass use_appname everywhere.

@kszys
Copy link
Contributor Author

kszys commented Sep 4, 2024

So... Massimo - I created a separate PR ( #918 ) to propose changes to the URL helper. If you like the idea, the next step would be to modify Form, Grid, and Auth to always use URL helper when manipulating the URLs. Let me know what you think.

@mdipierro
Copy link
Contributor

mdipierro commented Sep 4, 2024 via email

@kszys
Copy link
Contributor Author

kszys commented Sep 6, 2024

Assuming the Ombott issue described in #918 is patched, the current URL helper implementation should be sufficient to do what is needed to handle in py4web multi-app deployments with serving multiple domains in a clean way.

The next stage would be then to fix Form, Grid, and Auth to always use URL helper to manipulate the urls. I will see if, I can propose some PRs for that.

As such, this PR can be closed as not-applicable anymore since the solution using URL helper everywhere will be cleaner and more future-proof.

@kszys kszys closed this Sep 6, 2024
@kszys kszys deleted the grid_url_fix branch September 21, 2024 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants