search

wagtail / wagtailtrans

A Wagtail add-on for supporting multilingual sites
http://wagtailtrans.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
104 stars 60 forks source link

Wagtail 2.10 request.site is deprecated. #185

Closed DIGIREN closed 3 years ago

DIGIREN commented 4 years ago

In wagtail 2.10, request.site is deprecated and must be replaced with Site.find_for_request(request). In most cases it still works, but I've encountered a couple random scenarios where the use of request.site in wagtailtrans.middleware.TranslationMiddleware has resulted in WSGI errors. I've fixed this myself in my project by copying translationmiddleware into my own file and swapping the request.site for Site.find_for_request(request), but this will need to be fixed in order to maintain compatibility with 2.10.

As far as I can tell, this is the only incompatibility with 2.10 so far, ive been using wagtailtrans on it for a couple days now and i've been doing some testing and have had no issues except the wsgi errors caused by that incompatibility. The package will also need updated to be compatible with <=wagtail 2.10 as right now it is set at <wagtail 2.10, and starting in October, pip will use a new dependency resolver that does not allow installing a package with incompatible dependencies.

If its acceptable to you guys, I could probably have a PR for this in like a week if I get the time, if someone else wants to hop on it faster though by all means go for it, these changes shouldn't be terribly hard to make as far as i can tell.

jjanssen commented 4 years ago

Hi @DIGIREN, could you provide some more details on what issues you encountered with WSGI? Like for example which WSGI server and version are you running and what exact errors showed up when running? Did the errors also occur when running ./manage.py runserver or just specifically in a WSGI environment.

DIGIREN commented 4 years ago

Hey @jjanssen, it has been a little while at this point, so i dont 100% remember how I caused it but i know i was using Gunicorn and it was only occurring specifically within the WSGI environment. I should have time in a few days to boot back into my environment, unpatch it, and re-create the error. Then I can update you with more specific details :)

jjanssen commented 4 years ago

In addition #188 got merged and published so wagtailtrans is 'officially' compatible with Wagtail 2.10 which solves #186
Although I wasn't able to reproduce the above scenario during the update of the testsuite or briefly running gunicorn against our sandbox.

The deprecation of the SiteMiddleware itself shouldn't be official until the next Wagtail release (2.11). So I'm a bit confused why it is already causing troubles at this stage. I'm keeping this open in the meantime if anyone is able to share more details.

I've also opened a work-in-progress PR (#191) to keep track of the forthcoming 2.11 release. The changes in the Translation middleware are aimed to be backwards compatible for Wagtail 2.7 (LTS). But can be copied to provide an alternate solution for whoever is encountering the same problem until we are able to solve it.

jjanssen commented 3 years ago

Small update on this. We've released 2.2.1 this week and instead of waiting along on the official deprecation, we've taken into consideration that some are already leaving the SiteMiddleware prior to 2.11 due to the deprecation warning. So with 2.2.1 locally added middlewares can be removed and added again through Wagtailtrans.