Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add global block types #7229

Merged
merged 8 commits into from
Feb 21, 2024
Merged

Conversation

chirimoya
Copy link
Member

@chirimoya chirimoya commented Dec 9, 2023

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Fixed tickets fixes #6381
Related issues/PRs none
License MIT
Documentation PR sulu/sulu-docs#787

What's in this PR?

Global block types handling

Why?

Reduces size of block structure metadata and make it easier to reuse block structures.

Example Usage

<block name="blocks" default-type="text_block" minOccurs="0">
    <types>
        <type ref="text_block"/>
        <type name="gallery">
            <properties>
                <property name="images" type="media_selection"/>
            </properties>
        </type>
    </types>
</block>

To Do

  • Functionality
    • Read Blocks
    • Write Blocks
    • Block Title (getMeta?)
    • JSON Validation (mandatory fields)
  • Get feedback (@alexander-schranz, @mamazu)
  • Finalize implementation
  • Create a documentation PR
  • Add tests

To Disucuss

  • Naming "global-block" => "global-type"
  • Configuration in the XML

@chirimoya chirimoya added Feature New functionality not yet included in Sulu To Discuss The core team has to decide if this will be implemented labels Dec 9, 2023
@chirimoya chirimoya force-pushed the feature/global-block-types branch 5 times, most recently from 1552282 to e010abd Compare December 9, 2023 13:22
@mamazu
Copy link
Contributor

mamazu commented Dec 9, 2023

It was that easy? :D

But as for the implemetation, I'd suggest using a different tag name for this. This will then also allow us to expand this to other stuff as well.

@chirimoya chirimoya force-pushed the feature/global-block-types branch 3 times, most recently from 7cd6b9a to 2a73baf Compare December 10, 2023 21:53
@mamazu
Copy link
Contributor

mamazu commented Dec 15, 2023

Just tried this out in the dev environment only globalizing one block (or biggest content block) and these are the results:

Metric Before After
File Size 61.6MB 13.49MB
Compressed 1.51 MB 343KB
Time for getting the metadata 5sec 1sec

PS: The measurements are done with beautified json so most of the config is tabs or spaces.

@wachterjohannes
Copy link
Member

wachterjohannes commented Jan 26, 2024

i would suggest as the configuration:

<block name="blocks" default-type="text_block" minOccurs="0">
    <types>
        <type ref="text_block"/>
        <type name="gallery">
            <properties>
                <property name="images" type="media_selection"/>
            </properties>
        </type>
    </types>
</block>

which means that the reference is always a global block and without a name the name will be used by the global block but this name can be overwritten in the tag by adding a name attribute.

@wachterjohannes
Copy link
Member

Additionally is this not only a thing of of the block content type. We could also use it for other types which uses the types/type definition (image_map) for example.

But then the name "global-block" does not match anymore. I would suggest to rename this feature to "global-type". which is nearer a naming where it is used

@wachterjohannes
Copy link
Member

@alexander-schranz @chirimoya we have to discuss the open points next week

@alexander-schranz alexander-schranz added this to the Release 2.6 milestone Feb 5, 2024
@alexander-schranz alexander-schranz removed the To Discuss The core team has to decide if this will be implemented label Feb 5, 2024
@wachterjohannes
Copy link
Member

@mamazu i have finalized the implementation (added the validation) and also changed the configuration a bit! can you maybe test again this branch?

@mamazu
Copy link
Contributor

mamazu commented Feb 8, 2024

Well, one thing I found out during the testing is that you can not have xi:includes in your new block definition:

[ERROR 1871] Element 'xi:include': This element is not expected. (in /var/www/htdocs/config/templates/blocks/flexbox_row.xml - line 15, column 0)

Code:

<?xml version="1.0" ?>
<properties xmlns="http://schemas.sulu.io/template/template"
            xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
            xmlns:xi="http://www.w3.org/2001/XInclude"
            xsi:schemaLocation="http://schemas.sulu.io/template/template http://schemas.sulu.io/template/template-1.0.xsd"
>
    <block name="blocks" default-type="flexbox_row">
        <types>
            <type name="flexbox_row">
                <meta>
                    <title>Flexbox row</title>
                </meta>
                <properties>
                    <xi:include href="../blocks/fragments/padding_responsive.xml" xpointer="xmlns(sulu=http://schemas.sulu.io/template/template)xpointer(/sulu:properties/sulu:section)"/>
                    <property name="full_container" type="checkbox" colspan="4">
                        <meta>
                            <title>Full flexbox row container</title>
                        </meta>

                        <params>
                            <param name="type" value="checkbox"/>
                        </params>
                    </property>

                    <property name="collapse_margin" type="checkbox" colspan="4">
                        <meta>
                            <title>Collapse margin</title>
                        </meta>

                        <params>
                            <param name="type" value="checkbox"/>
                        </params>
                    </property>

                    <property name="color" type="color" colspan="4">
                        <meta>
                            <title>ui.color</title>
                        </meta>
                    </property>

                    <block name="flex_box" default-type="flex_box_block">
                        <xi:include href="../blocks/misc/flex_box_types.xml" xpointer="xmlns(sulu=http://schemas.sulu.io/template/template)xpointer(/sulu:properties/sulu:block/sulu:types)"/>
                    </block>
                </properties>
            </type>
        </types>
    </block>
</properties>

@alexander-schranz
Copy link
Member

alexander-schranz commented Feb 8, 2024

@mamazu The example looks unexpected from my point of view. I did not have a deeper look yet at this implementation but my expection is that a new block type inside config/templates/blocks looks like this with <template> root key:

<?xml version="1.0" ?>
<template xmlns="http://schemas.sulu.io/template/template"
          xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
          xsi:schemaLocation="http://schemas.sulu.io/template/template http://schemas.sulu.io/template/template-1.0.xsd">

    <key>flexbox_row</key>

    <meta>
        <title lang="en">Flexbox Row</title>
        <title lang="de">Flexbox Reihe</title>
    </meta>

    <properties>
                    <xi:include href="../blocks/fragments/padding_responsive.xml" xpointer="xmlns(sulu=http://schemas.sulu.io/template/template)xpointer(/sulu:properties/sulu:section)"/>
                    <property name="full_container" type="checkbox" colspan="4">
                        <meta>
                            <title>Full flexbox row container</title>
                        </meta>

                        <params>
                            <param name="type" value="checkbox"/>
                        </params>
                    </property>

                    <property name="collapse_margin" type="checkbox" colspan="4">
                        <meta>
                            <title>Collapse margin</title>
                        </meta>

                        <params>
                            <param name="type" value="checkbox"/>
                        </params>
                    </property>

                    <property name="color" type="color" colspan="4">
                        <meta>
                            <title>ui.color</title>
                        </meta>
                    </property>

                    <block name="flex_box" default-type="flex_box_block">
                        <xi:include href="../blocks/misc/flex_box_types.xml" xpointer="xmlns(sulu=http://schemas.sulu.io/template/template)xpointer(/sulu:properties/sulu:block/sulu:types)"/>
                    </block>
    </properties>
</template>

Where xincludes should work the same as for articles and snippets already as metadata should still be loaded the same way.

@mamazu
Copy link
Contributor

mamazu commented Feb 8, 2024

But with @alexander-schranz code I get a different error:
Attempted to call an undefined method named "getTypes" of class "Sulu\Bundle\AdminBundle\Metadata\FormMetadata\SectionMetadata".
Did you mean to call "getType"?

Same error message as in the pipelines.

image

@alexander-schranz
Copy link
Member

alexander-schranz commented Feb 8, 2024

The example looks like that your xincludes xmls live inside the config/templates/blocks directory but they should not be in that directory. It is expected that there now only live block templates. Like the pages only have page templates and articles only articles templates.

Like in the sulu demo I recommend you to move the xincluded xmls to something like config/templates/includes so they are not part of the templates blocks directory.

https://github.com/sulu/sulu-demo/tree/master/config/templates

Still there could be an error with that visitor.

@wachterjohannes
Copy link
Member

@mamazu yes there was an issue with the visitor. I think you have a section somewhere in the template! I have not tested it with it. But the last push should fix that. // cc @alexander-schranz

@wachterjohannes wachterjohannes force-pushed the feature/global-block-types branch 6 times, most recently from f1c41df to 219fe99 Compare February 9, 2024 08:04
@wachterjohannes wachterjohannes marked this pull request as ready for review February 9, 2024 08:05
@wachterjohannes
Copy link
Member

@alexander-schranz this is ready now :)

@alexander-schranz alexander-schranz removed their request for review February 9, 2024 09:09
@alexander-schranz
Copy link
Member

@chirimoya will do the review here.

@mamazu
Copy link
Contributor

mamazu commented Feb 9, 2024

Looks good from my side too.

@Prokyonn Prokyonn enabled auto-merge (squash) February 21, 2024 09:56
@Prokyonn Prokyonn merged commit da1403d into sulu:2.6 Feb 21, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New functionality not yet included in Sulu
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants