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

EZP-20097 Specify a api to field types for dealing with relation handling #201

Merged
merged 26 commits into from Feb 17, 2013

Conversation

pspanja
Copy link
Contributor

@pspanja pspanja commented Jan 14, 2013

This PR implements issue EZP-20097.

External storage is not used for storing relations anymore, instead a new method, getRelations(), is added to the FieldType interface and used from service to process and store Field relations (LINK, EMBED and FIELD types). For this an internal service RelationProcessor is implemented.

Also, ContentHandler::removeRelation() gets a second parameter $type, since Legacy Stack stores Content level relations (LINK, EMBED and COMMON types) into a same row using bitmask. Legacy Stack is capable of processing non-merged entries, so it is not required to update it not to use relation type bitmask. In time second parameter $type should be deprecated.

Alternatively it should also be possible to patch LS not to store relations with bitmask and thus avoid second parameter to removeRelation() (not probable anyone depends on ezcontentobject_link.id since they change with every update).

Linked with this PR:
ezsystems/ezpublish-legacy#529
ezsystems/ezoracle#13

{
// Using bitwise AND as Legacy Stack stores COMMON, LINK and EMBED relation types
// in the same entry using bitmask
if ( $relation->type & Relation::LINK )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the same think is done for LINK and EMBED, this could be handled with a unique condition:

if ( $relation->type & ( Relation::LINK | Relation::EMBED ) )

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, fixed in 6ec5602

@andrerom
Copy link
Contributor

Beside the comment by Patrick, big +1

}

// Remove relations not present in input set
foreach ( $mappedRelations as $relationType => $relationData )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: I would probably have written this with a unique loop and a switch statement to decrease cyclomatic complexity and increase clarity.

    // Remove relations not present in input set
    foreach ( $mappedRelations as $relationType => $relationData )
    {
        foreach ( $relationData as $relationEntry )
        {
            switch ( $relationType )
            {
                case Relation::FIELD:
                    foreach ( $relationEntry as $relation )
                    {
                        $this->persistenceHandler->contentHandler()->removeRelation(
                            $relation->id,
                            $relationType
                        );
                    }
                    break;
                case Relation::LINK:
                case Relation::EMBED:
                    $this->persistenceHandler->contentHandler()->removeRelation(
                        $relationEntry->id,
                        $relationType
                    );  
            }
        }
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in 83a5e07 :)

@patrickallaert
Copy link
Contributor

Besides my nitpicking, huge +1 :)

@pspanja
Copy link
Contributor Author

pspanja commented Jan 23, 2013

Thanks for the review guys!

@andrerom
Copy link
Contributor

So everything ready? :)

@pspanja
Copy link
Contributor Author

pspanja commented Jan 28, 2013

Nope :)

There are few fails with stubs, I need to update that first.

Conflicts:
	eZ/Publish/Core/FieldType/Relation/RelationStorage.php
	eZ/Publish/Core/FieldType/Relation/RelationStorage/Gateway.php
	eZ/Publish/Core/FieldType/Relation/RelationStorage/Gateway/LegacyStorage.php
	eZ/Publish/Core/FieldType/RelationList/RelationListStorage/Gateway/LegacyStorage.php
Conflicts:
	eZ/Publish/Core/FieldType/Tests/RelationTest.php
	eZ/Publish/Core/Repository/ContentService.php
@andrerom
Copy link
Contributor

Is it ready now? :)

( You should use sub tasks list to signal readiness of PR ;) see: https://github.com/blog/1375-task-lists-in-gfm-issues-pulls-comments )

@pspanja
Copy link
Contributor Author

pspanja commented Feb 16, 2013

Yes it's ready, but linked PRs on ezpublish-legacy and ezoracle should go together with it.

@andrerom
Copy link
Contributor

Ok, good you mentioned that, ezsystems/ezpublish-legacy#529 seems to need a update to be mergable.

@pspanja
Copy link
Contributor Author

pspanja commented Feb 17, 2013

Ok, I just rebased it.

andrerom added a commit that referenced this pull request Feb 17, 2013
EZP-20097 Specify a api to field types for dealing with relation handling
@andrerom andrerom merged commit bdbe486 into master Feb 17, 2013
@andrerom andrerom deleted the EZP-20097-RefactorRelations branch February 17, 2013 11:40
@andrerom
Copy link
Contributor

Thanks, it's now all in :)

*
* Example:
* <code>
* array(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The data structure does not look very clean. Nested structure can better be realized using structs, which has the benefit of autocompletion and better code checks in IDEs and tools. Also, having multiple possible values for a single key should be considered bad practice. Decide for a single, flexible format instead and stick to it.

ViniTou pushed a commit that referenced this pull request Feb 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants