Exclude not implemented templates for workspace / theme #1278

wachterjohannes · 2015-06-22T12:34:10Z

tasks:

informations:

q	a
Fixed tickets	fixes sulu/sulu-standard#202
BC breaks	templates are filtered by now => UPGRADE.md
Documentation PR	none

dantleech · 2015-06-23T05:51:44Z

As mentioned yesterday, if I understand coorectly, I do not think that associating XML structures with Webspaces via. Twig templates associated with Themes is a good idea. The template should know to which webspaces it is related, it should not b inferred indirectly.

It would be better imo to achieve this via a webspace whitelist in the XML structure:

<template>
    <webspace>sulu_io</webspace>
    <webspace>foobar</webspace>
    <!-- ... -->
</template>

The abscence of any webspaces in the XML template would imply that it should be available in all webspaces.

/cc @chirimoya @danrot

wachterjohannes · 2015-06-23T05:56:41Z

@dantleech i have ask @chirimoya for that and he thinks that it should work automatically!

danrot · 2015-06-23T06:04:25Z

@dantleech For me the templates are the more standalone part, so if we wouldn't make it automatic I would add this whitelist (or blacklist, I would offer both) to the webspace. But the automatic solution is fine for me.

dantleech · 2015-06-23T06:11:09Z

But this seems to me to be a very tenuous link. The association is implicit. Explicit support should be implemented first, and any "magic" should be secondary.

Templates are not absolutely a required part of a structure (we could imagine that templates are determined by other means). So, using the template, could prove to be flawed in the future.

In addition, "automatic" should mean "automatic configuration". The system should not be based on magic, but could use magic for configuration. Although I would not worry about such things at this stage.

I do not, however, believe in magic. :)

wachterjohannes · 2015-06-29T07:20:02Z

src/Sulu/Component/Content/Compat/Structure/StructureBridge.php

@@ -124,7 +120,7 @@ public function getWebspaceKey()
     */
    public function getUuid()
    {
-        return $this->getDocument()->getUuid();
+        return $this->getDocument()->getUuid($this->document);


i think this call cannot work before! the required argument $document was not given ...

Are you sure there is not just an incorrect type hint? I guess $this->getDocument() is already returning $this->document, so what's the point in passing it again?

And the method doesn't take an argument as you can see in https://github.com/sulu-io/sulu/blob/ca0f510f63386763564b99bafbfd45bad5e7a76d/src/Sulu/Bundle/ContentBundle/Document/BasePageDocument.php#L402

ou your rigth ... i will fix it

dantleech · 2015-06-30T06:10:10Z

Again, I don't see that this is a good thing as I understand it. Firstly, the current situation is not wrong imo. There is no relation between (stucture) templates and webspaces, it is OK in most cases that they show all (structure) templates in all webspaces. Secondly, a whitelist could be used to limit which templates are available in the webspace XML file. This is an incredibly simple solution to the problem and it gives the developer explicit control when they need it.

The "magic" approach will confuse people. If I add a (structure) template I expect it to be listed. If I add a template with a "wrong" Twig template, I would not think that would affect if it would be listed. With this PR I would add a template and it would just "not be there" and I would have no idea for why.

Or maybe I misunderstand..

danrot · 2015-06-30T06:31:47Z

The current situation is wrong for sure. If you have a multiple webspace setup you will have all the templates in both webspaces, and the content manager is able to choose some templates which might not be targeted at the current webspace. That's one of the biggest issues we currently have.

For me it would also be ok to have some kind of included and excluded tags on the xml, but I would definitely see this on the webspace, and not on the template.

dantleech · 2015-06-30T06:42:08Z

Also, what would happen if you wanted to use the same structure in two webspaces but with different Twig templates?

And yeah, I agree the whitelist would be in the webspace XML definition. It should say, I am the webspace, and these are my structure types (templates). If it is not defined then all templates would be available.

chirimoya · 2015-06-30T07:05:07Z

With the webspace theme you have configured the structures already "pre-selected". So for me adding all implemented templates (structures) to the workspace is kind of redundant and error prone.

Also in this case I would prefer convention of configuration.

chirimoya · 2015-06-30T07:06:01Z

+1

dantleech · 2015-06-30T07:36:20Z

Hmm, I guess my objection about using the structure in two webspaces with different Twig templates is not justified, as the Twig tempate is already hard-coupled to the structure type template.

I guess I will rest my case. However, I think this problem could be solved in a better way at a higher level. But we will see. +0.

danrot · 2015-06-30T07:37:40Z

No, it is already (and will still be) possible to use the same structure in two different webspaces with different twig templates. Actually the LiipThemeBundle is doing that for us.

danrot · 2015-06-30T07:45:07Z

@wachterjohannes This doesn't work, for all node related controller actions I get an error:

Document has not been applied to structure yet, cannot retrieve data from structure.

dantleech · 2015-06-30T09:34:12Z

On Tue, Jun 30, 2015 at 12:45:08AM -0700, Daniel Rotter wrote:

[1]@wachterjohannes This doesn't work, for all node related controller
actions I get an error:

Document has not been applied to structure yet, cannot retrieve data from structure.

It should probably now be adjusted to use the StructureMetadataFactory (sulu_content.structure.metadata_factory) to get
the metadata of the structures, assuming you do not need an actual
Document.

—
Reply to this email directly or [2]view it on GitHub.

Reverse link: [3]unknown

References

Visible links

https://github.com/wachterjohannes

Exclude not implemented templates for workspace / theme #1278 (comment)

Exclude not implemented templates for workspace / theme #1278 (comment)

wachterjohannes · 2015-06-30T09:56:10Z

@danrot @dantleech this is fixed (= that was my fault!

dantleech · 2015-06-30T10:49:13Z

src/Sulu/Bundle/CoreBundle/Resources/config/content.xml

+
+        <service id="sulu.content.webspace_structure_provider" class="%sulu.content.webspace_structure_provider.class%">
+            <argument type="service" id="twig"/>
+            <argument type="service" id="sulu.content.structure_manager"/>


Note that this is deprecated. We shoud be using the structure.metadata_factory or the document_manager depending on the use case.

yeah but i have implemented this before and dont want to change that much!

@dantleech btw, do all these classes have a deprecated annotation with a comment which class should be used instead?

danrot · 2015-07-01T08:07:29Z

It would be nice if you would use the SymfonyCache, so that the cache will be automatically refreshed when one of the template files changes.

wachterjohannes added the in progress label Jun 22, 2015

wachterjohannes changed the title ~~implemented webspace structure provider~~ Implemented webspace structure provider Jun 22, 2015

wachterjohannes self-assigned this Jun 22, 2015

wachterjohannes added this to the Release 1.0 milestone Jun 22, 2015

wachterjohannes changed the title ~~Implemented webspace structure provider~~ Exclude not implemented templates for workspace / theme Jun 22, 2015

wachterjohannes added review and removed in progress labels Jun 22, 2015

wachterjohannes mentioned this pull request Jun 23, 2015

Added default-template to webspace theme #1281

Merged

3 tasks

wachterjohannes force-pushed the feature/exclude-not-implemented-templates branch 2 times, most recently from 4fa0a73 to 2a26ea5 Compare June 29, 2015 06:14

wachterjohannes reviewed Jun 29, 2015
View reviewed changes

wachterjohannes force-pushed the feature/exclude-not-implemented-templates branch from 10910eb to 9c022ba Compare June 30, 2015 07:30

dantleech reviewed Jun 30, 2015
View reviewed changes

wachterjohannes force-pushed the feature/exclude-not-implemented-templates branch from a92d444 to 21535aa Compare July 1, 2015 06:58

wachterjohannes added 13 commits July 1, 2015 12:50

implemented webspace structure provider

b7148b9

added service and use it in template controller

fc39817

added message to validate command and added upgrade section

449f79e

added changelog

7546c62

added dist files

dc00ac7

removed excluded from webspace theme

f48804a

fixed webspace configs of testcases

6fc241b

fixed changelog

bc93351

fixed webspace structure provide testcases

9499311

fixed some strange syntax errors

ba05f02

fixed code style

8e1250b

fixed conditions in structure bridge

5413764

fixed theme testcases

409ed06

wachterjohannes force-pushed the feature/exclude-not-implemented-templates branch from f172765 to 409ed06 Compare July 1, 2015 10:51

danrot merged commit 409ed06 into develop Jul 1, 2015

danrot removed the review label Jul 1, 2015

danrot deleted the feature/exclude-not-implemented-templates branch July 1, 2015 10:56

Exclude not implemented templates for workspace / theme #1278

Exclude not implemented templates for workspace / theme #1278

Uh oh!

Conversation

wachterjohannes commented Jun 22, 2015

Uh oh!

dantleech commented Jun 23, 2015

Uh oh!

wachterjohannes commented Jun 23, 2015

Uh oh!

danrot commented Jun 23, 2015

Uh oh!

dantleech commented Jun 23, 2015

Uh oh!

wachterjohannes Jun 29, 2015

Choose a reason for hiding this comment

Uh oh!

danrot Jun 30, 2015

Choose a reason for hiding this comment

Uh oh!

danrot Jun 30, 2015

Choose a reason for hiding this comment

Uh oh!

wachterjohannes Jun 30, 2015

Choose a reason for hiding this comment

Uh oh!

dantleech commented Jun 30, 2015

Uh oh!

danrot commented Jun 30, 2015

Uh oh!

dantleech commented Jun 30, 2015

Uh oh!

chirimoya commented Jun 30, 2015

Uh oh!

chirimoya commented Jun 30, 2015

Uh oh!

dantleech commented Jun 30, 2015

Uh oh!

danrot commented Jun 30, 2015

Uh oh!

danrot commented Jun 30, 2015

Uh oh!

dantleech commented Jun 30, 2015

Uh oh!

wachterjohannes commented Jun 30, 2015

Uh oh!

dantleech Jun 30, 2015

Choose a reason for hiding this comment

Uh oh!

wachterjohannes Jun 30, 2015

Choose a reason for hiding this comment

Uh oh!

danrot Jul 1, 2015

Choose a reason for hiding this comment

Uh oh!

danrot commented Jul 1, 2015

Uh oh!

Uh oh!