Produits Entr'ouvert » Hobo

Ça continue, du coup j'ai créé une branche dédiée, http://git.entrouvert.org/hobo.git/log/?h=wip/django-1.11

À noter qu'elle commande par #15797 pour ne pas devoir rebaser sans, relecture à commencer par là, donc.

À noter maintenant en plus ce commit : multitenant: update template loader for django 1.11 (#20933)

D'une part il vire la compatibilité Django < 1.8 pour simplifier les loaders (et le CachedLoader de beaucoup notamment parce qu'il y a maintenant upstream une méthode cache_key qu'il suffit de redéfinir) (mais on n'utilise il me semble nulle part ce CachedLoader, qui est du code de 2014, présent depuis le début de hobo/multitenant).

Plus important, il adapte le FilesystemLoader suite à des évolutions dans Django 1.9 (la possibilité de loader récursif, ce qui permet à un template "x.html" de faire un extends de "x.html", plus loin dans la chaine de découverte, ce qui est pas mal utile) et Django 1.10 (la possibilité de chemin relatif); le loader récursif, il permettrait à variants/grandlyon-gnm/combo/page_template.html de simplement faire extends combo/page_template.html, plutôt que extends ../../combo/page_template.html, et la gestion des chemins relatifs, elle casse le ../../combo/page_template.html. Bref, comme on est encore avec django 1.8 on ne peut pas adapter publik-base-theme et donc ça contourne l'affaire, il y a un commentaire dans le code pour expliquer ça.

Via le nouveau commit "multitenant: clear internal settings cache when changing tenant" on découvre le ticket https://code.djangoproject.com/ticket/27625 qui optimise l'accès aux settings de Django et se faisant casse cette partie du multitenant; du coup au traitement d'une requête (via le middleware) et dans la commande tenant_command sur l'appel avec --all-tenants, ajout du code pour vider le cache interne de django.conf.settings. (idéalement ça serait dans connection.set_tenant() pour être sûr que ça se trouve bien appelé partout mais je n'étais pas tenté par le monkeypatch ici.

Au final ça laissait quand même les usuels problèmes d'accès concurrents du coup j'ai plutôt opté pour le monkepatch de settings.__getattr__, pour retourner à une version sans cache.

• un « DjancoCachedLoader » (s/co/go/) à renommer dans multitenant/template_loader.py
• toujours dans template_loader.py, au lieu de template_name = template_name[9:] if template_name.startswith('variants/') else template_name ligne 62, je préfèrerais
  

      if template_name.startswith('variants/'):
          template_name = template_name[9:]
  

  
  parce que bon.
• est-ce qu'on ne mettrait pas cette gestion de variants/ dans le if hasattr(settings, 'TEMPLATE_VARS') and settings.TEMPLATE_VARS.get('theme'): situé quelques lignes plus bas ? (pas vraiment d'impact car hobo n'est pas utilisé en dehors de Publik ; c'est juste pour ranger un peu)
• pour le LazySettings.__getattr__ est-on sûr qu'on veut autant simplifier la fonction ? Je n'arrive pas à voir si on aurait besoin de la partie if self._wrapped is empty: self._setup(name) (cf https://github.com/adamchainz/django/blob/1.11.2/django/conf/__init__.py#L55). Je serais plus tranquille à reprendre le getattr en retirant uniquement le cache :
  

       def __getattr__(self, name):
           if self._wrapped is empty:
               self._setup(name)
           return getattr(self._wrapped, name)
  

• Et allez, j'ai jamais osé faire chier avec ça, mais « Inline comments should be separated by at least two spaces from the statement » (et là mon vi me crie dessus depuis que j'ai un peu mis à jour mon vérificateur pep8). Donc au lieu de :
  

    Origin = None # django < 1.9
  

  
  C'est :
  

    Origin = None  # django < 1.9
  

  
  (c'est dernier point est vraiment trèèès important et trèèès intéressant)

• un « DjancoCachedLoader » (s/co/go/) à renommer dans multitenant/template_loader.py

Corrigé.

• toujours dans template_loader.py, au lieu de template_name = template_name[9:] if template_name.startswith('variants/') else template_name ligne 62, je préfèrerais
  [...]
  parce que bon.

Ok, changé.

• est-ce qu'on ne mettrait pas cette gestion de variants/ dans le if hasattr(settings, 'TEMPLATE_VARS') and settings.TEMPLATE_VARS.get('theme'): situé quelques lignes plus bas ? (pas vraiment d'impact car hobo n'est pas utilisé en dehors de Publik ; c'est juste pour ranger un peu)

Pour moi on rangera en virant simplement ce code quand on aura basculé pour de bon en 1.11, je le laisse du coup où il est.

• pour le LazySettings.__getattr__ est-on sûr qu'on veut autant simplifier la fonction ? Je n'arrive pas à voir si on aurait besoin de la partie if self._wrapped is empty: self._setup(name) (cf https://github.com/adamchainz/django/blob/1.11.2/django/conf/__init__.py#L55). Je serais plus tranquille à reprendre le getattr en retirant uniquement le cache :

Quelques lignes plus haut on fait conf.settings._wrapped = settings.TenantSettingsWrapper(conf.settings._wrapped) et ça nous assure que _wrapped existe bien.

• Et allez, j'ai jamais osé faire chier avec ça, mais « Inline comments should be separated by at least two spaces from the statement » (et là mon vi me crie dessus depuis que j'ai un peu mis à jour mon vérificateur pep8). Donc au lieu de :

Modifié mais je ne promets pas de faire gaffe à ça tout le temps.

(tout ça poussé dans la branche wip/)

Frédéric Péters a écrit :

Quelques lignes plus haut on fait conf.settings._wrapped = settings.TenantSettingsWrapper(conf.settings._wrapped) et ça nous assure que _wrapped existe bien.

Ok dans ce cas faisons le conf.LazySettings.__getattr__ = lambda ... dans le if qui contient ça (ligne 111) ?

Ok dans ce cas faisons le conf.LazySettings.__getattr__ = lambda ... dans le if qui contient ça (ligne 111) ?

Branche modifiée pour faire ça.

J'ai aussi fait des tests avec le CachedLoader, désormais testé et modifié. (et poussé la branche à jour)

MAIS il ne fonctionne pas avec Django 1.11 tant qu'on a des templates qui font {% extends "../../même-nom" %} (parce qu'on altère le nom du template pour gérer ça). (c'est dans trois intégrations graphiques qui ne sont pas sur notre SaaS d'ailleurs).

Une fois qu'on aura basculé sur Django 1.11, on pourra modifier publik-base-theme pour faire {% extends "même-nom" %} et adapter le FilesystemLoader pour faire :

-            known_template_names = ['variants/%s/%s' % (theme_value, template_name), template_name]
+            known_dirnames = ['%s/variants/%s' % (x, theme_value) for x in known_dirnames] + known_dirnames
+            known_template_names = [template_name]

(et en fait on pourra nettoyer davantage)

J'ai poussé wip/django-1.11-tox, tiré à partir de wip/django-1.11, qui fait passer les tests au vert (sauf ceux de tests_authentic qui ont besoin d'un authentic compatible django 1.11). J'ai modifié essentiellement les tests. Et quelques lignes de debian_config_common.

En attendant #21489 et django-tenant-schemas, je fais aussi vivre cette branche dans mon environnement local.

Merci j'ai intégré tes modifications; ça m'irait bien maintenant sans attendre les dépendances de tirer ça dans master, sans déclarer 1.11 dans tox pour le moment. Ça serait ok pour toi ?

Et je viens d'envoyer dans https://git.entrouvert.org/hobo.git/log/?h=wip/django-1.11 ce que je propose pour master.

C'est bon pour moi !

Petit hoquet sur les tests de theme_base mais c'est corrigé.