Opened 13 months ago

Closed 4 months ago

#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.)

Change History (13)

comment:1 Changed 7 months ago 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 7 months ago by jsonesen

  • Owner set to jsonesen

comment:3 follow-up: Changed 6 months ago 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 6 months ago by juliandoucette (previous) (diff)

comment:4 in reply to: ↑ 3 Changed 6 months ago 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 6 months ago by kvas

  • Priority changed from Unknown to P3
  • Ready set

comment:6 Changed 6 months ago by kvas

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

comment:7 Changed 6 months ago by jsonesen

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

comment:8 Changed 6 months ago 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 6 months ago by juliandoucette

What do you guys think?

Option 3 SGTM.

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

comment:10 Changed 6 months ago by kvas

  • Description modified (diff)

I updated the issue text based on comment 9.

comment:11 Changed 5 months ago by kvas

  • Blocked By 6605 added

comment:12 Changed 5 months ago by abpbot

comment:13 Changed 4 months ago by jsonesen

  • Resolution set to fixed
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.