Handle the dependency on plone.app.standardtiles

[ale-rt]

Problem: #94 introduced an hard dependency to plone.app.standardtiles that is not reflected in the setup.py.

The package soft-depended on plone.app.standardtiles in 5.0.1.

ale@avo:~/Code/plone/projects/coredev/5.2/src/plone.app.blocks$ git grep standardtiles 5.0.1
5.0.1:plone/app/blocks/subscribers.py:        if "plone.app.standardtiles.field" in tile_url:
5.0.1:plone/app/blocks/tests/test_contentlayout.py:  <div data-tile="./@@plone.app.standardtiles.html/example"
...

After #94 we have an hard dependency:

ale@avo:~/Code/plone/projects/coredev/5.2/src/plone.app.blocks$ git grep standardtiles 5.1.0
5.1.0:plone/app/blocks/linkintegrity.py:from plone.app.standardtiles import html
5.1.0:plone/app/blocks/linkintegrity.py:from plone.app.standardtiles import existingcontent
5.1.0:plone/app/blocks/subscribers.py:        if "plone.app.standardtiles.field" in tile_url:
5.1.0:plone/app/blocks/tests/test_contentlayout.py:  <div data-tile="./@@plone.app.standardtiles.html/example"
...

plone.app.standardtiles already depends on plone.app.blocks, so we have a circular dependency.

Note: #94 is still not merged in master

[ale-rt]

It seems that the proper thing is that plone.app.standardtiles should depend on plone.app.blocks, and not viceversa.

[ale-rt]

CC @gotcha, @mauritsvanrees , @jensens

Do you have some suggestions?

@petschki already shared some thoughts on #96.

[jensens]

Probably the linkintegrity code was just added in the wrong package? That said I reviewed #94 and did not find this problem....

[gotcha]

I am also surprised. I do not understand at first sight how this circular import problem is not triggered in our setup.

Now, I dug a bit: I would argue that it feels semantically right that p.a.blocks depends on p.a.standardtiles in the sense that blocks is the integrating glue. (Opposite to @ale-rt opinion).

If sthing needs to be improved, it is the implementation of existincontent tile: it makes sense that it wants to include the rendered blocks, but this imo leads to move existingcontent from standardtiles to blocks, thus breaking the circular dependency.

I look forward to pursuing this conversation.

[jensens]

I would argue that it feels semantically right that p.a.blocks depends on p.a.standardtiles in the sense that blocks is the integrating glue.

Blocks should not need p.a.astandardtiles. The p.a.standardtiles README also states:

"This package contains standard tiles to be used with Plone Mosaic (or as such with Plone Blocks composition)." https://github.com/plone/plone.app.standardtiles/blob/master/README.rst

OTOH existingcontent.py is the only place where p.a.blocks is used. But I do not see a simple way to refactor it.

Anyway, looking at #94 I propose to move .linkintegrity.ExistingContentTile and its registration from p.a.blocks to p.a.standardtiles existingcontent.py file.

[mauritsvanrees]

The way I have it in my head, I would say that plone.app.blocks is a basic building block, part of the tile infrastructure. plone.app.standardtiles stands on top of that. So plone.app.blocks should not depend on plone.app.standardtiles. I did not notice this either when reviewing the PR.

I would advise to move the adapters for html and existing content tile to plone.app.standardtiles, and keep the rest in here. Does that make sense? @gotcha Could you do that?

[gotcha]

@petschki @ale-rt @mauritsvanrees

I agree that the linkintegrity implementation of html and existingcontent should be moved to plone.app.standardtiles and will work on it.

Nevertheless, I'd appreciate your insights on the following point:

I just tried running a 5.2.9 buildout with plone.app.mosaic. I could check that link integrity is working the way we intended. But I do not see any import problems. IOW, I cannot trigger the circular import issue.

And I really would like to understand why.

Can you tell me how you trigger that issue ? Do you understand why it does not trigger with a basic Plone 5.2.9 ?

[ale-rt]

The package that triggered the error depends only on plone.app.blocks.

It does not need neither p.a.mosaic nor p.a.standardtiles.

That's why I got the error in the first place and that's also probably the reason you did not notice it: you probably checked the patch on a site with plone.app.mosaic and for that reason you did not see the missing dependency.

[gotcha]

@ale-rt Makes sense. In the case you mention, do you see a circular import issue or a missing import ?

I still do not have a clue why the circular import issue does not show up with p.a.mosaic.

[ale-rt]

Circular imports are not a problem, what I see is a missing dependency, see: https://community.plone.org/t/plone-5-2-9-released/15481/3?u=alert

[gotcha]

@ale-rt Makes sense. I got distracted to think about a circular import issue by not understanding properly @petschki https://github.com/plone/plone.app.blocks/pull/94#issuecomment-1192313685.

[mauritsvanrees]

I have made these two PRs to move code from plone.app.blocks to plone.app.standardtiles for Plone 5.2:

https://github.com/plone/plone.app.blocks/pull/99 https://github.com/plone/plone.app.standardtiles/pull/136

[ale-rt]

This is fixed now by the PRs mentioned by Maurits. Thanks!