Sulu: Allow to segment entire webspace into different URLs

[danrot]

Problem description

It would be nice if the entire webspace could be segmented into two different "versions" of the website. That is useful especially for seasonal websites; many tourism websites for e.g. skiing areas have dedicated pages for winter and summer, because they offer different leisure activities based on the time of the year.

Proposed solution

This feature is similar to the already existing AudienceTargeting feature, the main exception being that the current segment is not chosen by some rules, but solely determined by the URL. This means that the following features are required:

• [x] Recognize the segment from cookie
• [x] Allow to assign segments in the excerpt tab of pages
• [x] Smart Content must not return pages not being part of the segment passed via the URL
• [x] Allow to assign segments to page blocks
• [x] Only provide the blocks to the template that match the current segment
• [x] Redirect URLs without segments to the default segment (No redirect necessary because cookies are used)
• [x] Add a dropdown in the preview toolbar to watch the website in different segments
• [x] Consider the segment when generating URLs for the sitemap (Not necessary because URL stays the same, since cookies are used now)
• [x] Correctly generate URLs including the segment (sulu_content_path, navigation, page selection) (Since cookies are used the URLs are always the same)
• [x] Add parameter to navigation twig extensions for segments
• [ ] Cache pages with segments correclty using the Cookie

Open questions

• [x] How to determine which segment is the default (for the skiing area case mentioned above that should probably change based on the time of the year)
  -> see https://github.com/sulu/sulu/issues/5271#issuecomment-628474230, there is only the default in the XML, has to be changed manually
• [ ] Should there be a switch in the Smart Content configuration to tell if the segment should be considered?
• [x] What about blocks that have no segments assigned? Are they visible for all segments or for none of them?
  -> Will be visible in all segments
• [x] Should it also be possible to throw 404s when the passed segment in the URL does not exist?
  -> segments are not part of the URLs for now
• [x] Should we maybe include the segment in the URL only if the segment changes the content? Would avoid duplicate content issues, but requires us to remember the current segment in a different way, e.g. in a cookie. Alternatively we could always have the segment in the URL, but would have to be careful when setting canonical links. In that case it might also be problematic if the default segment changes every 6 months and therefore also the canonical tag changes.
  -> We go even a step further, and never pass the segment in the URL. A cookie will be used instead. That has the adavantage that we don't have to worry about duplicate content at all.

[danrot]

I've started to implement this, and began with the assignment of pages in the excerpt tab. I have already got it working with a configuration like this:

        <property name="ext/excerpt/segments" type="select">
            <meta>
                <title>sulu_admin.segments</title>
            </meta>
            <params>
                <param
                    name="values"
                    type="expression"
                    value="service('sulu_admin.segment_select_helper').getValues(webspace)"
                />
            </params>
        </property>

I've had to do some adjustments, but now you can use the metadataOptions that are passed via the form request as variable in expression params.

But now the segments field is always shown, even if the passed values are empty. That looks weird, and I would argue that the field should not be displayed if the values are empty. But I don't see a mechanism we have already implement that would support that.

There is only the visibleCondition, which is used to do that using an expression language being passed to the frontend. In the frontend we only pass the the data of the current form and the locale using the __locale key. The data would even include the webspace the page is part of, but I can't figure out solely on the webspace name if the webspace has some segments or not.

Now the question is how we get this information, resp. how we want to configure that in the XML. Something that comes to my mind is this:

<property name="ext/excerpt/segments" type="select" visibleCondition="__webspace.segments.length &gt; 0">
    <!-- ... -->
</property>

The __locale attribute has been added inline to this object, but I would argue that it should not be done like that for the __webspace. I would say there should be the possibility to add a function to a registry, that will receive the data and return what should be appended. Then the PageBundle can add a function that will add the current webspace object as __webspace. This way that mechanism is easily extensible with other properties as well, and we don't have to add webspace specific logic to the AdminBundle.

[danrot]

The suggestion in the previous comment could also be used to only display the shadowPage if the currently selected webspace has more than one language. If we do that, this feature could be easily extracted into a separate PR.

The only question that is left IMO is how we name the registry the previously mentioned functions for adding e.g. the __webspace to the data being passed to e.g. the visibleCondition. Since the variable is called evaluationData I would suggest the name EvaluationDataEnhancerRegistry. WDYT?

[nnatter]

When reading the proposal yesterday, I thought about the term ConditionDataProvider. On one hand, we (I :wink:) used provider for the function that provides item-actions to the List container. On the other hand, we already use the term Condition in the developer-facing interfaces 🙂

[danrot]

Ok, I think I am going with ConditionDataProvider then :slightly_smiling_face:

[danrot]

One task has been solved, now the next question comes up :slightly_smiling_face: Since some decisions are yet to be made about how to handle URLs and stuff like that, I'd like to continue with the block settings, because the requirements there are pretty clear. However, the question is how to we represent these block settings in the API and in PHPCR. I basically see three options here:

1. We introduce a new node called settings/configuration or however, and make this a reserved keyword for blocks. This would looks something like this in the API:

{
    "blocks": {
        "title": "Test title",
        "type": "default",
        "configuration": {
            "segment": "summer"
        }
    }
}

In PHPCR that would probably look like that:

i18n:en-blocks-type#0: "default"
i18n:en-blocks-title#0: "Test title"
i18n:en-blocks-configuration#0-segment: "summer"

1. We split data and configuration completely, which would also free us from having any naming conflicts:

{
    "blocks": {
        "type": "default",
        "data": {
            "title": "Test title",
        },
        "configuration": {
            "segment": "summer"
        }
    }
}

This change could probably also be done without any restructuring of the data in PHPCR, if we make configuration a keyword that cannot be used as name within a block (then the data in PHPCR would look exactly the same as in 1).

Would have the advantage that we already have split that in the API, and it might be easier to reimplement that without this naming conflict e.g. in Sulu 3.0.

1. Same as option 2, but already change the structure of the data in PHPCR. Would be a BC break, but therefore there would be no naming conflicts.

The API is also the same as in option 2, but the PHPCR structure would have to be changed to something like this:

i18n:en-blocks-type#0: "default"
i18n:en-blocks-data-title#0: "Test title"
i18n:en-blocks-configuration#0-segment: "summer"

WDYT?

[alexander-schranz]

@danrot I would go with settings maybe we should prefix it with _settings. And I would make it just a keyword without wrapping all in a data key make migrations a lot simplier also for article and content bundle or any other custom bundle. So for me it should look like:

{
    "title": "Test title",
    "type": "default",
    "_settings": {
        "disabled": true,
        "segment": "summer"
    }
}

[danrot]

The point I was trying to make is, that we could also go for a hybrid solution :slightly_smiling_face: That was basically option 2. We keep the data structure in PHPCR as is, and let the content type handle the distinction between what is data and settings. AFAIK that would than also work with the article bundle, but I don't know about the content bundle.

I think when rewriting everything I would go for an approach like this, because it allows us to avoid all naming conflicts. And doing so at least in the API now would maybe save us some troubles when we introduce something like that.

But I think I also won't go for option 3 because of BC issues. However, I would also be fine with option 1. We have already some keys in this API with a prefixed _, which mean something like "not direct content, but some supporting role", which I am not sure these settings would also qualify. They are something that can be directly manipulated in the UI, and using underscores for them somehow feels weird to me for that reason. But maybe I am the only one :see_no_evil:

Oh, and I am also wondering if we should use settings or configuration... When adding this to the Block react component, it would feel more sound to have callbacks called onRemove and onConfigure. If we name that settings I could only come up with onSettingsClick, and then it is weird that it is not also called onRemoveClick :see_no_evil: But I think using onSettingsClick and onRemove is also fine.

[alexander-schranz]

We should definitly not change the location of the type and the other fields like title, they need to stay on the block root level. Adding logic in the content type (hybrid solution) does have no effect on article bundle index, content bundle or a custom entity as the content types are not used there. Also changing the block data would break all custom apis using a form with block in it (e.g. form bundle). So what is send to the api and the with what the field type work need to stay in the following format else an update with custom entities using blocks is horror:

"blocks": [{
    "type": "test",
    "title": "test"
}]

So we should just add a settings/configuration to it, if we do not prefix it with _ is ok for me.

So Option 1 I think is the only thing we can go for.

[danrot]

The next problem is a side problem of the block settings. The question is how do we know which form should be displayed in the settings overlay.

At first I quickly thought it might make sense to use the property children approach, which would allow to put properties below other properties. I thought @alexander-schranz created a issue regarding that proposal, but I can't find it anymore. That would be something our CardCollection field type already supports for the addresses and bank information, but only hard coded, and not yet via XML configuration. But then I realized that this is not really logical to use, because the blocks already have child properties, namely the properties appearing within the block.

So I think I would still stick with the variant of passing the form key to the block settings:

<block name="blocks" default-type="editor" minOccurs="0">
    <meta>
        <title lang="en">Content</title>
    </meta>
    <params>
        <param name="settings_form_key" value="page_block_settings" />
    </params>
    <types>
        <!-- ... -->
    </types>
</block>

Mind that this param will not have to be set manually for every page, it will be appended there automatically. But using this param it can be overriden for e.g. custom entities, so that the block settings could show something different there.

However, that raises the question how to load the schema for the page_block_settings form. Sadly we've been a bit inconsistent here. The MemoryFormStore takes a schema as constructor parameter, whereby the ResourceFormStore takes a formKey and loads the schema asynchronously.

For the current use case it would be nice to have the MemoryFormStore, but with the behavior of the ResourcFormStore, i.e. loading the schema from the server.

After thinking a bit about it, I would probably not pass the formKey to the ResourceFormStore if I would implement it again. I would also only pass the schema, and let something else load the schema from the server. The way I am currently imagining that, I would probably use a Factory for that. I know we can't implement that now for the ResourceFormStore, because that would be a huge BC break. But we could do something like that for the MemoryFormStore:

const memoryFormStore = memoryFormStoreFactory.createFromFormKey(formKey, data);

If somebody wants to pass the schema in the code directly we could simply use the MemoryFormStore constructor, or a different method on the memoryFormStoreFactory (would even prefer the latter, because it would allow us to be less coupled to the constructor):

const memoryFormStore = memoryFormStoreFactory.createFromSchema(schema, jsonSchema, data);

Now it might feel weird to have this for the MemoryFormStore, but not for the ResourceFormStore... Since I have not seen yet a case where a ResourceMemoryFormStore or something like that would have been useful, I would say that it would be OK to not have a resourceFormStoreFactory at the moment.

So I would propose to create only create the memoryFormStoreFactory for now (open for better naming suggestions), and live with the fact that it is not possible to have a ResourceFormStore with a complete custom schema defined on the client side.

Jotted it down quite quickly, in order to get your feedback, and don't forget my thoughts until tomorrow, because I want definitely to consult my pillow on that :see_no_evil:

[alexander-schranz]

@danrot Just more a question a typically toolbar action with form currently look like this in my projects:

    componentWillUnmount() {
        if (this.filterFormStore) {
            this.filterFormStore.destroy();
        }
    }

    getFilterFormStore() {
        // will create filter form store only when really needed
        if (!this.filterFormStore) {
            this.filterFormStore = new ResourceFormStore(
                new ResourceStore('campaign_email_filters'), 'campaign_email_filters'
            );
        }

        return this.filterFormStore;
    }

    getNode() {
        // shorten
        return (
            this.showDialog && !this.listStore.selectionIds.length ? <Form
                onSubmit={this.handleConfirm}
                store={this.getFilterFormStore()}
            /> : null
    );
}

As you see there is the destroy called on component unmount is that still needed for the memory form store? If yes would it be possible to provide instead a render able component with some change function which handle that e.g:

    getNode() {
        return (
            this.showDialog && !this.listStore.selectionIds.length ? <MemoryForm
                key='campaign_email_filters'
                data={this.data}
                onChange={this.onChange}
            /> : null
    );

Just some idea I came apart. Else your suggestion above looks good for me.

[danrot]

Hm... I would say it would come with some downsides.

Passing the data like you've suggested in your last example would mean that you don't have any access to the FormStore at all, right? That would mean that you would always have to implement the dirty mechanism on your own, instead of using store.dirty to recognize if something changed. There are also a bit more information you would not be able to access, e.g. the occured errors or if it is currently loading.

On the other handside we could pass the FormStore to the MemoryForm and still destroy it automatically when the MemoryForm unmounts, but I think that is not something the developer would expect to happen. He never called destroy on the FormStore, but for some reason it might still stop working.

That being said, we probably need the existing way of doing anyway for some cases, and I am not a huge fan of introducing too many ways of doing things. So I would suggest we go for the factory approach, and see how that works out. In case we say it is still cumbersome for some reason, we can still later introduce a MemoryForm.

Regarding my initial approach:

The example for memoryFormStoreFactory.createFromFormKey would probably not be correct, because this code needs to send a request, and therefore must return a promise. So it should probably look something like this:

memoryFormStoreFactory.createFromFormKey(formKey, data).then(action((memoryFormStore) => {
    // assuming that this is an observable, so that the form can be rendered after the store has been initialized
    this.memoryFormStore = memoryFormStore; 
}));

It is a little bit harder to read than the one above, but I think there is not really a way around this... But it would still save us from having to know which stores to call and how the results have to be passed to the store. See e.g. the example in our ProfileFormOverlay:

https://github.com/sulu/sulu/blob/edbf2d56fd7843ee6775ee47a5a0e394bb4fe708/src/Sulu/Bundle/AdminBundle/Resources/js/containers/ProfileFormOverlay/ProfileFormOverlay.js#L31-L43

So I think this would still be of some value. But I am not sure where I would put this. Should we introduce a new factories folder for stuff like that in every bundle that needs it? Or should we just place the factory right next to the store it is creating? Once we have that decided, I think I could implement that for the ProfileFormOverlay in a separate PR and reuse it in #5277 afterwards.

I would also especially be interested in the opinion of @nnatter, because he asked to improve that situation :slightly_smiling_face:

[nnatter]

As far as I understand, the proposed solution would allow me to replace this code snippet:

Promise.all([ 
    metadataStore.getSchema(FORM_KEY), 
    metadataStore.getJsonSchema(FORM_KEY), 
    ResourceRequester.get(RESOURCE_KEY), 
]).then(action([schema, jsonSchema, data]: [RawSchema, Object, Object]) => {
    this.formStore = new MemoryFormStore(data, schema, jsonSchema);
})); 

with this:

memoryFormStoreFactory.createFromFormKey(formKey, data).then(action((formStore) => {
    this.formStore = formStore; 
}));

I definitely think this would be an improvement. But I am not sure if it is a big enough improvement to introduce a (new) factory concept. Firstly, as far as i know, we dont really have something similar to the memoryFormStoreFactory in the existing codebase, right? Secondly, as you already said, there would be no resourceFormStoreFactory, which makes it special even inside of the Form namespace.

Also, there is another thing that i am not sure about. With the proposed solution, the developer still needs to handle the asynchronicity of loading the schema of the form key. One of the things I like about our other stores (eg the ResourceFormStore) is that you can just construct them and directly pass them to the component that needs them. This way the receiving component can decide what should be rendered when the store is loading. With the proposed solution, each parent component that would like to render a Form with a MemoryFormStore needs to implement what is rendered while the schema is loaded by itself.

After thinking a bit about it, I would probably not pass the formKey to the ResourceFormStore if I would implement it again. I would also only pass the schema, and let something else load the schema from the server.

Is there a specific reason for this feeling? Would it be okay for you to pass something like a FormMetadataStore (kind of similar to the resourceStore parameter) instead of the raw schema? That way, the user would not need to handle the asynchronicity.

For the current use case it would be nice to have the MemoryFormStore, but with the behavior of the ResourcFormStore, i.e. loading the schema from the server.

Unfortunately, we cannot change the existing formstores because of BC reasons. But maybe we could just provide a third FormStore type with this behaviour? Something like a DataFormStore with a constructor similar to the ResourceFormStore that accepts the raw data instead of a ResourceStore. Sure enough its hard to find a good name for that and it is not a perfect solution either, but we would not need to introduce a new concept in the codebase.

[danrot]

I definitely think this would be an improvement. But I am not sure if it is a big enough improvement to introduce a (new) factory concept. Firstly, as far as i know, we dont really have something similar to the memoryFormStoreFactory in the existing codebase, right? Secondly, as you already said, there would be no resourceFormStoreFactory, which makes it special even inside of the Form namespace.

I think it is a bigger improvement than it looks on first sight. It does not save a lot of lines, but it frees the developer from quite some stuff:

• No need to know from where to import the metadataStore
• No need to know which functions from the metadataStore has to be called
• No need to know how to pass which of the results from the metadataStore to the MemoryFormStore

That would also allow us to change more stuff, without breaking existing applications (as long as we update our own Factory accordingly. This frees the mind for quite some other tasks.

When thinking about it, we could even introduce a ResourceFormStoreFactory only offering methods using the formKey, because the ResourceFormStore does not allow another form of instantiation currently. A createFromFormKey method could also instantiate the ResourceStore for you and pass it correctly to the ResourceFormStore, while a createFromResourceStore method would allow reusing an existing ResourceStore.

In general I think that the problem (especially with the current situation around the MemoryFormStore here) is mainly how to object construction, which is the job of a factory. Therefore I think it is a great fit, and would be a worthwhile addition to our tool belt.

Also, there is another thing that i am not sure about. With the proposed solution, the developer still needs to handle the asynchronicity of loading the schema of the form key. One of the things I like about our other stores (eg the ResourceFormStore) is that you can just construct them and directly pass them to the component that needs them. This way the receiving component can decide what should be rendered when the store is loading. With the proposed solution, each parent component that would like to render a Form with a MemoryFormStore needs to implement what is rendered while the schema is loaded by itself.

Yeah, I also thought of that as the biggest downside of this approach. That could maybe be mitigated by making the Factory return a decorated MemoryFormStore, which should work quite nicely, because we have already have a FormStoreInterface in place. The decorated store could then handle the asynchronicity and we could still pass that to the Form.

I think what kept me from suggesting that, is that it already feels weird to me now, because the loading flag of the FormStore could mean two different things (loading data or loading schema). I have to have a closer look at that, but it is easily possible that this is an implementation detail, which didn't concern developers only using these components.

After thinking a bit about it, I would probably not pass the formKey to the ResourceFormStore if I would implement it again. I would also only pass the schema, and let something else load the schema from the server.

Is there a specific reason for this feeling? Would it be okay for you to pass something like a FormMetadataStore (kind of similar to the resourceStore parameter) instead of the raw schema? That way, the user would not need to handle the asynchronicity.

Yeah, the reason is that we have two stores that behave differently, although they could behave exactly the same. That also gets more obvious to me now, when we want the MemoryFormStore to behave more like the ResourceFormStore, so this shared behavior (loading the schema from the server) should live somewhere outside both of these stores.

I would prefer a Factory over adding another store to the constructor, because using it is a lot easier. Compare the following code snippets:

// Assuming that the store is somehow decorated instead of returned as a promise
const memoryFormStore = memoryFormStoreFactory.createFromFormKey(formKey, data);

const memoryFormStore = new MemoryFormStore(
    data,
    new FormMetadataStore(/* whatever we have to pass in here */)
);

The developer would still have to know about multiple classes in order to construct a memoryFormStore, whereby in the first example only knowledge about the memoryFormStoreFactory is required. That would get even more complicated for the ResourceFormStore example.

For the current use case it would be nice to have the MemoryFormStore, but with the behavior of the ResourcFormStore, i.e. loading the schema from the server.

Unfortunately, we cannot change the existing formstores because of BC reasons. But maybe we could just provide a third FormStore type with this behaviour? Something like a DataFormStore with a constructor similar to the ResourceFormStore that accepts the raw data instead of a ResourceStore. Sure enough its hard to find a good name for that and it is not a perfect solution either, but we would not need to introduce a new concept in the codebase.

Naming is really the main reason I am afraid of going down this path. What should be the difference between the DataFormStore and the MemoryFormStore? Judging by the name they don't seem to be doing anything different. And apart from the fact that they are instantiated differently, they probably also would do exactly the same. I am repeating myself, but that's the main reason I think a Factory would be a great fit here. Who knows, maybe there are more features to come that require us to share some cross-cutting concerns among different stores. Then a factory might be of great help.

TL;DR: I have the same concerns about returning a promise from the factory, but I think that can be solved by decorating the store in some way. Would still use a Factory instead of creating more different FormStore implementations.

[danrot]

@nnatter @alexander-schranz Started an unfinished implementation at https://github.com/sulu/sulu/pull/5282, it is already working for the ProfileFormOverlay. Let's continue our discussion there.

[danrot]

Now I am about to implement the serverside part of the segments, and also here some questions are coming up:

1. The way we have decided to implement it now, means that we don't need the segmentKey on the read and write method of the ContentTypeInterface as it currently is. The content manager is not able to manage completely different content for each segment as it is done for the locale, instead only some content types (smart content and blocks) filter the content automatically, based on information the content manager provides. AFAICT this means that the segmentKey is required on the getContentData and getViewData methods of the ContentTypeInterface.

2. Shall we only use the default attribute of the segment in order to find out which cookie to set by default? And if a tourism website wants to change between winter and summer a developer has to change the default attribute of the segments in the XML manually?

3. How do we change between different segments using the segments chooser? Do we provide some query parameter, that will cause the server to return the other segment and a Set-Cookie header? Or should we leave that responsibility on the client side? I.e. set the cookie using JS and reload the page manually if required?

4. I guess the segment recognition should be implemented in a separate SegmentRequestProcessor reading the values from a cookie or set the default segment if no cookie is available.

[danrot]

I've had a call with @alexander-schranz and @chirimoya where we have been talking about these points, adding a summaray below:

1. The way we have decided to implement it now, means that we don't need the segmentKey on the read and write method of the ContentTypeInterface as it currently is. The content manager is not able to manage completely different content for each segment as it is done for the locale, instead only some content types (smart content and blocks) filter the content automatically, based on information the content manager provides. AFAICT this means that the segmentKey is required on the getContentData and getViewData methods of the ContentTypeInterface.

We decided that we leave the ContentTypeInterface untouched, because it would be quite a big BC Break, and the segment is only required by very few content types. Instead we want to correctly implement the getSegment method of the RequestAnalyzer and inject it in the content types that need this information.

1. Shall we only use the default attribute of the segment in order to find out which cookie to set by default? And if a tourism website wants to change between winter and summer a developer has to change the default attribute of the segments in the XML manually?

The default attribute will be changed manually when seasons change. We don't add any more functionality to support the developer in that regard (at least not currently, who knows what the future brings).

1. How do we change between different segments using the segments chooser? Do we provide some query parameter, that will cause the server to return the other segment and a Set-Cookie header? Or should we leave that responsibility on the client side? I.e. set the cookie using JS and reload the page manually if required?

The current idea is to implement a custom controller handling URLs like /segment_switch?segment=w&url=/activities.

An alternative would be to add a segment query parameter to the current URL, and handle that query parameter in Varnish resp. in the Symfony HTTP Cache. This would probably result in a better performance, but we said that it is probably not worth the additional effort of implementing that twice for both HTTP Cache implementations.

1. I guess the segment recognition should be implemented in a separate SegmentRequestProcessor reading the values from a cookie or set the default segment if no cookie is available.

Nobody had any concerns in that regard.