Opened on 09/04/2017 at 11:43:51 AM

Closed on 05/15/2018 at 08:50:50 PM

#5618 closed change (fixed)

Localized image paths should fall back to unlocalized paths using CMS test server

Reported by: juliandoucette Assignee: jsonesen
Priority: P3 Milestone: Websites continuous integration
Module: Sitescripts Keywords:
Cc: kvas, jsonesen, wspee, ire Blocked By: #6605
Blocking: Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29723671

Description (last modified by kvas)

Background

If we use relative image paths (img/example.png instead of /img/example.png) then CMS will localize them automatically (/en/img/example.png). But they will not resolve properly unless the image is placed in all relevant locales directories (locales/en/img/example.png, locales/de/img/example.png, etc).

What to change

The CMS should be smarter about resolving the links:

  • Before a link to /$locale/$path is inserted into the page we should check if such URL exists.
  • In case it doesn't, but /$default_locale/$path does, the link should be rewritten to that (this is already implemented).
  • If the locale-specific paths don't exist but /$path exists (coming from /static/$path), the link should be rewritten to this (this is the functionality that needs to be added).

(This should also work with siteurl so that we can deploy websites to non-root directories.)

Attachments (0)

Change History (13)

comment:1 Changed on 03/08/2018 at 07:49:27 PM by jsonesen

So, basically we want the CMS to no longer localize images, or to use non localized images in the case where the expected image is not in the locale directory?

Or would a config option be preferable, which indicates whether resources should be automatically localized?

comment:2 Changed on 03/08/2018 at 07:49:41 PM by jsonesen

  • Owner set to jsonesen

comment:3 follow-up: Changed on 03/12/2018 at 01:24:02 PM by juliandoucette

So, basically we want the CMS to no longer localize images, or to use non localized images in the case where the expected image is not in the locale directory?

The latter.

Use cases.

  1. If I place an image in locales/$defaultlocale/ and not /locales/de/ then request de/$image then I should receive $defaultlocale/$image
  2. If I place an image in static/ and not /locales/de/ then request de/$image then I should receive static/$image

Assuming:

  • Request: /de/$image
  • That locales/de/$image does not exist

The logic should work as follows:

  1. Check locales/de/$image
    1. Y: Serve
    2. N: Check locales/$defaultlocale/$image
      1. Y: Serve
      2. N: Check static/$image
        1. Y: Serve
        2. N: 404

(This is all suggestion. Please suggest otherwise if you know better.)

Last edited on 03/12/2018 at 01:25:02 PM by juliandoucette

comment:4 in reply to: ↑ 3 Changed on 03/13/2018 at 01:25:06 AM by jsonesen

Replying to juliandoucette:

Use cases.

  1. If I place an image in locales/$defaultlocale/ and not /locales/de/ then request de/$image then I should receive $defaultlocale/$image
  2. If I place an image in static/ and not /locales/de/ then request de/$image then I should receive static/$image

Assuming:

  • Request: /de/$image
  • That locales/de/$image does not exist

The logic should work as follows:

  1. Check locales/de/$image
    1. Y: Serve
    2. N: Check locales/$defaultlocale/$image
      1. Y: Serve
      2. N: Check static/$image
        1. Y: Serve
        2. N: 404

(This is all suggestion. Please suggest otherwise if you know better.)

I think it is simple enough to change the test server for this behavior. Thanks for the suggestion, what do you think Vasily?

Also, since the nginx server behaves this way I think it shouldn't be a problem to make this the default behavior of the test server.
If Vasily agrees we can ready this ticket and I can upload a patch.

comment:5 Changed on 03/13/2018 at 09:19:55 AM by kvas

  • Priority changed from Unknown to P3
  • Ready set

comment:6 Changed on 03/13/2018 at 09:21:57 AM by kvas

The proposal sounds good to me. Thanks for figuring out the details, guys.

comment:7 Changed on 03/15/2018 at 09:11:47 PM by jsonesen

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:8 Changed on 03/23/2018 at 07:23:06 PM by kvas

Hi guys, I started reviewing this today and I became more skeptical of the whole plan that we came up with. I owe you an apology -- I should have thought about it in more detail before, but better late then never, so here's it goes.

What we've decided to do (and what Jon has implemented) is to hardcode the following redirects into the test server:

  1. /$locale/img/... -> /img/... (if /$locale/img/... doesn't exist),
  2. /$locale/img/... -> /$default_locale/img/... (if /$locale/img/... doesn't exist) -- this one is actually implemented as /$locale/... -> /$default_locale/....

The reason for that is that our nginx config does something similar. To be more precise, it has two redirects (one: $locale/... -> ... and two: /... -> /$default_locale/...). Redirect one produces the effect of (1) above and the combination of one and two produces (2).

I have two issues with this:

  1. We're hardcoding some details related to our web server config files into the test server. This makes CMS harder to use for anyone but us and even for us in different circumstances.
  2. We're hardcoding the img prefix, specific to how we structure some of our websites, so it's not even equivalent to our web server config. For example /de/some_image.png would get redirected to /some_image.png if the latter exists on our web server, but the test server would not redirect in this case.

Here are some, options on how we could make it better:

  1. Don't rely on img prefix, implement redirection as in nginx config. Still not so good for uses not related to our websites but at least there would be no discrepancy from the websites served through nginx. This one requires the least work.
  1. Make redirection configurable. Something like this perhaps:
    [missing_paths]
    rewrite =
        /../(.*) -> /en/\1
        /../(.*) -> /\1
    

Pretty cool and flexible, but more work and, to be frank, I'm not sure this rewriting functionality actually belongs in the test server.

  1. Make CMS resolve the links to static images better: if there's no /$locale/$image and no /$default_locale/$image but there's /$image (coming from /static folder in the website), the CMS would rewrite the link to point to /$image (and the test server would work the same way). This would not replicate the redirects of nginx in the test server but shared (between locales) images that work in production would also work with test server so it would solve the original problem of this ticket.

To me option 3 seems preferable because it goes to the root of the problem by fixing the broken links that require the redirects in nginx config. Or perhaps I don't understand the problem?

What do you guys think?

comment:9 Changed on 04/03/2018 at 11:19:14 AM by juliandoucette

What do you guys think?

Option 3 SGTM.

(Sorry for the delay. I was on vacation.)

comment:10 Changed on 04/04/2018 at 06:56:31 PM by kvas

  • Description modified (diff)

I updated the issue text based on comment 9.

comment:11 Changed on 04/25/2018 at 09:16:06 AM by kvas

  • Blocked By 6605 added

comment:12 Changed on 05/08/2018 at 05:37:57 PM by abpbot

comment:13 Changed on 05/15/2018 at 08:50:50 PM by jsonesen

  • Resolution set to fixed
  • Status changed from reviewing to closed

Add Comment

Modify Ticket

Change Properties
Action
as closed .
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from jsonesen.
 
Note: See TracTickets for help on using tickets.