search

web2py / py4web

Other
244 stars 126 forks source link

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

Closed kszys closed 6 days ago

kszys commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

Sorry if slow but this week I have limited internet. will be a lot more responsive next week.

On Mon, Aug 19, 2024, 20:06 Krzysztof Socha @.***> wrote:

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.

— Reply to this email directly, view it on GitHub https://github.com/web2py/py4web/pull/911#issuecomment-2297142042, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHLZTYKFIWEXNXEAQDKM23ZSIX3PAVCNFSM6AAAAABMXXUI76VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEOJXGE2DEMBUGI . You are receiving this because you commented.Message ID: @.***>

kszys commented 2 weeks ago

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 commented 2 weeks ago

i think this needs to be addressed in URL or the web servet. app should not need to know

On Mon, Aug 26, 2024, 05:53 Krzysztof Socha @.***> wrote:

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.

— Reply to this email directly, view it on GitHub https://github.com/web2py/py4web/pull/911#issuecomment-2310142899, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHLZT5LMQCJKQOWGAUWH6DZTMQN3AVCNFSM6AAAAABMXXUI76VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMJQGE2DEOBZHE . You are receiving this because you commented.Message ID: @.***>

kszys commented 2 weeks ago

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 commented 2 weeks ago

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.

kszys commented 2 weeks ago

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.

mdipierro commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

will review tonight. 👍

On Wed, Sep 4, 2024, 12:23 Krzysztof Socha @.***> wrote:

So... Massimo - I created a separate PR ( #918 https://github.com/web2py/py4web/pull/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.

— Reply to this email directly, view it on GitHub https://github.com/web2py/py4web/pull/911#issuecomment-2329807035, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHLZT7UIQWN3DQ7GY5EBADZU5MZJAVCNFSM6AAAAABMXXUI76VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMRZHAYDOMBTGU . You are receiving this because you commented.Message ID: @.***>

kszys commented 6 days ago

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.