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

Validate required fields in form blocks #4889

Merged
merged 6 commits into from Nov 14, 2019

Conversation

@danrot
Copy link
Member

danrot commented Nov 13, 2019

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

What's in this PR?

This PR considers the block content type when generating the JSON schema for a form.

Why?

Because this way the validation for required fields will also work in blocks.

Example Usage

        <block name="blocks" default-type="text" minOccurs="0">
            <meta>
                <title lang="de">Text</title>
                <title lang="en">Text</title>
            </meta>
            <types>
                <type name="text">
                    <meta>
                        <title lang="de">Text</title>
                        <title lang="en">Text</title>
                    </meta>
                    <properties>
                        <property name="title" type="text_line" mandatory="true">
                            <meta>
                                <title lang="de">Title</title>
                                <title lang="en">Title</title>
                            </meta>
                        </property>
                        <property name="editor" type="text_editor" mandatory="true">
                            <meta>
                                <title lang="de">Article</title>
                                <title lang="en">Article</title>
                            </meta>
                        </property>
                    </properties>
                </type>
                <type name="image">
                    <meta>
                        <title lang="de">Text</title>
                        <title lang="en">Text</title>
                    </meta>
                    <properties>
                        <property name="title" type="text_line" mandatory="true">
                            <meta>
                                <title lang="de">Title</title>
                                <title lang="en">Title</title>
                            </meta>
                        </property>
                        <property name="image" type="media_selection" mandatory="true">
                            <meta>
                                <title lang="de">Image</title>
                                <title lang="en">Image</title>
                            </meta>
                        </property>
                    </properties>
                </type>
            </types>
        </block>

BC Breaks/Deprecations

To discuss

@@ -11,49 +11,35 @@
namespace Sulu\Bundle\AdminBundle\Metadata\SchemaMetadata;
class PropertyMetadata
class PropertyMetadata extends AbstractPropertyMetadata

This comment has been minimized.

Copy link
@danrot

danrot Nov 13, 2019

Author Member

@sulu/core-team If I would do that one again, I would probably name it ConstMetadata. Initially I thought having one PropertyMetadata will be enough, but then we might end up with a huge constructor when adding more stuff to validate. I've already had the properties from the ArrayMetadata added here as well, but I decided to split these two.

Question is if we should rename this class (I would suggest so), and if we introduce a BC layer (Creating a new class called ConstMetadata extending from this one, and deprected the PropertyMetadata class).

I would go for the deprecation, and if we do that, I would first get rid of all deprecations that currently have no alternative (if there is no alternative it should not be a deprecation) and configure our ŧests to fail if we use some of our own deprecated code.

@danrot danrot force-pushed the danrot:bugfix/block-required-validation branch from 908d1b9 to b9af3ca Nov 13, 2019
@danrot danrot changed the title Validate required fields in form blcoks Validate required fields in form blocks Nov 13, 2019
@danrot danrot force-pushed the danrot:bugfix/block-required-validation branch from b9af3ca to 868e07e Nov 13, 2019
@danrot danrot force-pushed the danrot:bugfix/block-required-validation branch from 868e07e to a3f8c8c Nov 14, 2019
new SchemaMetadata([], $blockTypeSchemas)
);
}
if (!$itemMetadata->isRequired()) {

This comment has been minimized.

Copy link
@nnatter

nnatter Nov 14, 2019

Member

does it still make sense to return here? 🤔

This comment has been minimized.

Copy link
@danrot

danrot Nov 14, 2019

Author Member

No, I think that is not necessary anymore 🙂

class ConstMetadata extends PropertyMetadata
{
/**
* @var string|number|null

This comment has been minimized.

Copy link
@alexander-schranz

This comment has been minimized.

Copy link
@danrot

danrot Nov 14, 2019

Author Member

What do you want to say by that? A field in a form can return about anything, so we should at least cover these two (I don't imagine right now that people are using this in combination with objects as well).

This comment has been minimized.

Copy link
@alexander-schranz

alexander-schranz Nov 14, 2019

Member

Number does not exist as type in PHP. https://docs.phpdoc.org/guides/types.html
PHP only know about int|float

@danrot danrot merged commit 9afca66 into sulu:release/2.0 Nov 14, 2019
4 checks passed
4 checks passed
PrettyCI Code formatting
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@danrot danrot deleted the danrot:bugfix/block-required-validation branch Nov 14, 2019
@alexander-schranz alexander-schranz added this to the Release 2.0 milestone Nov 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.