sitemap changes and tests coming from plone.app.layout PR #92 #235

erral · 2016-06-03T08:42:39Z

No description provided.

…ll-request

See plone/plone.app.layout#91 and plone/plone.app.layout#92 Also see plone#209

erral · 2016-06-03T08:45:48Z

See plone/plone.app.layout#92 before merging.

djowett · 2016-06-03T11:09:29Z

@erral Ah, right I see your issue.

Then I think we should just add a pin in plone.app.multilingual/test-plone-4.3.x.cfg (rather than setup.py) because we are not trying to force PAM 1.x users to take our fix, we are only checking it doesn't break things for them.

But if you still think we should pin in setup.py, then I would do something like this here:

  requirements=[
    'setuptools',
    'five.grok',
    'plone.multilingual>=1.1',
    'z3c.relationfield',
    'plone.app.z3cform',
    'plone.app.registry',
    'plone.directives.form',
    'plone.formwidget.contenttree',
    'Products.PloneLanguageTool',
    'archetypes.multilingual < 2.0',  # required while archetypes is default in Plone
  ],

 # I left you this bit - cos I'm not sure how to do it (unless we can use plone.api?)
 plone_version = xxx.yyy
 if plone_version >= '4.3':   
     requirements.append('plone.app.layout > 2.3.14')

  ....
    install_requires=requirements,

erral · 2016-06-03T11:37:13Z

And how are we supposed to maintain compatibility with Plone 4.2 that uses p.a.layout 2.2.x ? 😵

djowett · 2016-06-03T12:31:00Z

@erral you've got me thinking, but do we really need to fix in p.a.layout 2.2.x ? ....

We are fixing an issue which occurs when using plone.app.multilingual > 2.x and above.
The fix needs to be in plone.app.layout branches that are used by Plone versions that use PAM > 2.x.
So the big question is:
Is PAM 2.x used by Plone 4.2 (or 4.1)? Are you doing that?

theoretically speaking:

We don't test PAM 2.x with anything lower than 4.3
Plone 4.2 is restricted to 1.0 of plone.app.contenttypes, does that version work with PAM 2.x?

so it's a narrow case

practically speaking then:
I think unless you or someone we know is using this combination, then I'd rather just add a note in the Issue / PR that a backport may be required if someone is in that circumstance. This is fun, but more than enough work :-)

erral · 2016-06-03T15:42:45Z

But my branch tests PAM 1.x which is available both for Plone 4.3.x and
Plone 4.2.x.

On Fri, Jun 3, 2016 at 2:31 PM, Daniel Jowett notifications@github.com
wrote:

@erral https://github.com/erral you've got me thinking, but do we
really need to fix in p.a.layout 2.2.x ? ....

We are fixing an issue which occurs when using plone.app.multilingual >
2.x and above.
The fix needs to be in plone.app.layout branches that are used by Plone
versions that use PAM > 2.x.
So the big question is:
Is PAM 2.x used by Plone 4.2 (or 4.1)? Are you doing that?

theoretically speaking:

We don't test PAM 2.x with anything lower than 4.3
https://github.com/plone/plone.app.multilingual/blob/2.x/.travis.yml#L13

Plone 4.2 is restricted to 1.0 of plone.app.contenttypes, does that
version work with PAM 2.x? so it's a narrow case

I think unless you or someone we know is using this combination, then
I'd rather just add a note in the Issue / PR that a backport may be
required if someone is in that circumstance. This is fun, but more than
enough work :-)

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#235 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAx41ZZLdUEWcV1bpwOs-4EwAFOsp8-cks5qIB6JgaJpZM4ItUj4
.

Mikel Larreategi
mlarreategi@codesyntax.com

CodeSyntax
Azitaingo Industrialdea 3 K
E-20600 Eibar
Tel: (+34) 943 82 17 80

djowett · 2016-06-03T16:13:26Z

I think I;m clear on this now....

But for PAM 1.x we only want to prove that it /doesn't make a
difference/ to the sitemap
which plone.app.layout they take (2.3.15 or earlier).
So we provide a pin in the 4.3.x test
(add plone.app.layout = 2.3.15 in test-plone-4.3.x.cfg)
but /not/ in the 4.2.x test

What we don't need to (or want to) do is force////people to take our
change when they are only using PAM 1.x, so we can remove the pin on
plone.app.layout from setup.py

I think that should work....

On 03/06/16 16:42, Mikel Larreategi wrote:

But my branch tests PAM 1.x which is available both for Plone 4.3.x and
Plone 4.2.x.

On Fri, Jun 3, 2016 at 2:31 PM, Daniel Jowett notifications@github.com
wrote:

@erral https://github.com/erral you've got me thinking, but do we
really need to fix in p.a.layout 2.2.x ? ....

We are fixing an issue which occurs when using plone.app.multilingual >
2.x and above.
The fix needs to be in plone.app.layout branches that are used by Plone
versions that use PAM > 2.x.
So the big question is:
Is PAM 2.x used by Plone 4.2 (or 4.1)? Are you doing that?

theoretically speaking:

We don't test PAM 2.x with anything lower than 4.3

https://github.com/plone/plone.app.multilingual/blob/2.x/.travis.yml#L13

Plone 4.2 is restricted to 1.0 of plone.app.contenttypes, does that
version work with PAM 2.x? so it's a narrow case

I think unless you or someone we know is using this combination, then
I'd rather just add a note in the Issue / PR that a backport may be
required if someone is in that circumstance. This is fun, but more than
enough work :-)

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub

#235 (comment),
or mute the thread

https://github.com/notifications/unsubscribe/AAx41ZZLdUEWcV1bpwOs-4EwAFOsp8-cks5qIB6JgaJpZM4ItUj4
.

Mikel Larreategi
mlarreategi@codesyntax.com

CodeSyntax
Azitaingo Industrialdea 3 K
E-20600 Eibar
Tel: (+34) 943 82 17 80

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#235 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABZuRSOkyktUiNPHN2ou12HfzMKZpvXwks5qIEt9gaJpZM4ItUj4.

Daniel Jowett

Jowett Enterprises Ltd
Tel: 0333 3 553 773
Mobile: 07870 667 126
daniel@jowettenterprises.com
www.jowettenterprises.com

djowett · 2016-12-15T23:22:57Z

Hi @erral, I've been taking another look at this & I'm coming to the conclusion that the sitemap is already broken on the PAM 1.x / Plone 4.2 combination. Do you agree?

https://travis-ci.org/plone/plone.app.multilingual/jobs/135087425 shows links not appearing in the sitemap, and I've tested this manually on a fresh buildout - it seems that documents in some languages (the non default ones?) get missed off the sitemaps....

It's surprising because I thought the sitemap is working for the combination of PAM 1.x & Plone 4.3 - but I better double check that too!

cc @Rudd-O

erral · 2016-12-16T08:18:02Z

I need to refresh this :)

The sitemap works for me on:

Plone 4.3 - PAM 1.x branch - p.a.layout 2.3.x_issue_91 branch

The sitemap works for me partially on:

Plone 4.2 - PAM 1.2.2 - p.a.layout 2.2.10.

With partially I mean that it works for the language folders, but it doesn't work for the whole site's sitemap, it only returns the contents of one language; presumably because the "language: 'all'" "hack" is missing.

erral · 2016-12-16T08:23:35Z

I think we should ignore the whole Plone 4.2 thing, just mark the next PAM 1.x release for Plone 4.3.x only, and let it go.

djowett · 2016-12-17T23:48:26Z

@erral Thanks - in the end I just skipped these new sitemap tests for Plone 4.2 - if someone wants to backport the fix into p.a.layout 2.2.x later then these tests would I guess (as you did above) be fixed. I just don't want to get any further into changing versions I'm not using at this time.

The fixed PR is #236. But before we merge it, I should get someone to merge and release my fix in p.a.layout

djowett · 2016-12-19T18:18:51Z

@erral I'm closing this as it is superceded by #236 - thanks for starting things off!

erral and others added 4 commits June 1, 2016 16:06

port the changes needed for p.a.multilingual from plone.app.layout pu…

f4d75d2

…ll-request

Add tests for sitemap.xml.gz (expect to fail for now)

a87c580

See plone/plone.app.layout#91 and plone/plone.app.layout#92 Also see plone#209

adapt the tests for PAM 1.x

7ebc52a

include p.a.layout branch in tests

34a7511

erral mentioned this pull request Jun 3, 2016

Rework sitemap.xml.gz view to fix it for plone.app.multilingual [2.3.x] plone/plone.app.layout#92

Merged

djowett closed this Dec 19, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sitemap changes and tests coming from plone.app.layout PR #92 #235

sitemap changes and tests coming from plone.app.layout PR #92 #235

erral commented Jun 3, 2016

erral commented Jun 3, 2016

djowett commented Jun 3, 2016 •

edited

Loading

erral commented Jun 3, 2016 •

edited

Loading

djowett commented Jun 3, 2016 •

edited

Loading

erral commented Jun 3, 2016

djowett commented Jun 3, 2016

djowett commented Dec 15, 2016

erral commented Dec 16, 2016

erral commented Dec 16, 2016

djowett commented Dec 17, 2016

djowett commented Dec 19, 2016

sitemap changes and tests coming from plone.app.layout PR #92 #235

sitemap changes and tests coming from plone.app.layout PR #92 #235

Conversation

erral commented Jun 3, 2016

erral commented Jun 3, 2016

djowett commented Jun 3, 2016 • edited Loading

erral commented Jun 3, 2016 • edited Loading

djowett commented Jun 3, 2016 • edited Loading

erral commented Jun 3, 2016

djowett commented Jun 3, 2016

djowett commented Dec 15, 2016

erral commented Dec 16, 2016

erral commented Dec 16, 2016

djowett commented Dec 17, 2016

djowett commented Dec 19, 2016

djowett commented Jun 3, 2016 •

edited

Loading

erral commented Jun 3, 2016 •

edited

Loading

djowett commented Jun 3, 2016 •

edited

Loading