EZP-31127: Skipped non-existing Content Types in ContentTypeIdentifier criterion #2872

emodric · 2019-11-14T09:17:31Z

Question	Answer
JIRA issue	EZP-31127
Bug/Improvement	yes
New feature	no
Target version	`7.5`
BC breaks	no
Tests pass	yes
Doc needed	no

Makes it possible to use non existent content type identifiers in the criterion (e.g. when coming from an outside DB field, where the content type might be deleted in the meantime).

Otherwise, you'd have to use ContentTypeId and load the IDs yourself.

TODO:

Implement feature / fix a bug.
Implement tests.
Fix new code according to Coding Standards ($ composer fix-cs).
Ask for Code Review.

emodric · 2019-12-13T13:32:28Z

Hi. Will this be going in any time soon?

It would be nice if it could make it into the next patch release.

eZ/Publish/Core/Search/Legacy/Content/Common/Gateway/CriterionHandler/ContentTypeIdentifier.php

emodric · 2020-01-08T08:27:15Z

@andrerom @adamwojs Updated the implementation to return a none-matching expression if there are no valid IDs.

andrerom · 2020-01-08T10:04:15Z

+1, but should ideally have some small integration test coverage for this in terms of legacy SQL vs Solr vs ...

emodric · 2020-01-14T14:38:30Z

Any tips on getting started with the test? As far as I can see, this does not have any tests at all.

andrerom · 2020-01-15T07:00:38Z

@emodric

ezpublish-kernel/eZ/Publish/API/Repository/Tests/SearchServiceTest.php

Lines 132 to 140 in 31bd435

    
           6 => [ 
        
               [ 
        
                   'filter' => new Criterion\ContentTypeIdentifier( 
        
                       'user' 
        
                   ), 
        
                   'sortClauses' => [new SortClause\ContentId()], 
        
               ], 
        
               $fixtureDir . 'ContentTypeId.php', 
        
           ],

Maybe duplicate this one with a invalid type? And duplicate the MatchNone case below with two invalid types.

emodric · 2020-01-15T10:10:40Z

Not sure what you mean about duplicating MatchNone. It does not have any arguments, no?

emodric · 2020-01-15T10:30:30Z

BTW, Solr variant of this PR: ezsystems/ezplatform-solr-search-engine#161

andrerom · 2020-01-15T13:05:01Z

Not sure what you mean about duplicating MatchNone. It does not have any arguments, no?

Assuming MatchNone fixture is for empty response here, but something like:

                [
                    'filter' => new Criterion\ContentTypeIdentifier(
                        'invalid1',
                        'invalid2'
                    ),
                    'sortClauses' => [new SortClause\ContentId()],
                ],
                $fixtureDir . 'MatchNone.php',
            ],

emodric · 2020-01-15T13:36:27Z

Ah, okay, I understand now. Updated the test case.

emodric · 2020-01-15T15:41:47Z

Failures are probably related to not having the changes from ezsystems/ezplatform-solr-search-engine#161.

alongosz · 2020-01-16T09:35:30Z

eZ/Publish/Core/Search/Legacy/Content/Common/Gateway/CriterionHandler/ContentTypeIdentifier.php

+            try {
+                $idList[] = $this->contentTypeHandler->loadByIdentifier($identifier)->id;
+            } catch (NotFoundException $e) {
+                // Skip non-existing content types


Question: wouldn't we actually want to log (warning) usage of non-existing Content Type identifiers? So the Maintainers can trace any outdated apps.

@andrerom mentioned it earlier. Yes, but as a separate improvement, that adds logging consistently to every handler that has the same issues.

For the case where only some are invalid I agree we should add warning directly here, as it's a bit unique case compared to other criteria.

Maybe something like:

Suggested change

// Skip non-existing content types

// Skip non-existing content types, but track for code below

$invalidIdentifiers[] = $identifier;

Then further down:

if (count($idList) === 0) { return $query->expr->not($query->bindValue('1')); } else (!empty($invalidIdentifiers)) { // log warning or error }

Updated in 0ef9b7f

emodric · 2020-01-22T10:00:05Z

Going in? :)

micszo · 2020-01-22T10:15:44Z

In queue for QA. Maybe I'll be able to start today. Aiming to get it in this week.

emodric · 2020-01-22T10:17:12Z

@micszo Thanks for the update! 👍

micszo · 2020-01-23T08:41:40Z

@emodric could you please try rebasing both PRs? some tests failed due to missing TranslatedName which was introduced in a fix several days ago.

…nt types

emodric · 2020-01-23T09:12:01Z

@micszo Done!

micszo

QA Approved on eZ Platform EE 2.5 with branches with Legacy SE & Solr.
In case of LSE warning is printed twice but we confirmed it's not related to these changes.
Thanks for support to @alongosz!

lserwatka · 2020-01-31T08:08:56Z

@alongosz could you merge it up?

alongosz · 2020-01-31T15:28:36Z

Merged up to master along with today's other changes as 717d488.

Thank you @emodric 🎉

emodric · 2020-01-31T15:45:12Z

Thanks all for review!

… converting content type identifiers to IDs Related issue fixed in eZ Publish kernel 7.5.7 ezsystems/ezpublish-kernel#2872

Fixed in eZ Publish kernel 7.5.7 ezsystems/ezpublish-kernel#2872

…tType() directly Fixed in eZ Publish kernel 7.5.7 ezsystems/ezpublish-kernel#2872

Related issue fixed in eZ Publish kernel 7.5.7 ezsystems/ezpublish-kernel#2872

Fixed in eZ Publish kernel 7.5.7 ezsystems/ezpublish-kernel#2872

emodric mentioned this pull request Nov 14, 2019

EZP-31127: Skipped non-existing Content Types in ContentTypeIdentifier criterion ezsystems/ezplatform-solr-search-engine#161

Merged

andrerom changed the title ~~Do not use non-existing content types in ContentTypeIdentifier criterion~~ EZP-31127: Do not use non-existing content types in ContentTypeIdentifier criterion Nov 25, 2019

adamwojs reviewed Dec 20, 2019

View reviewed changes

eZ/Publish/Core/Search/Legacy/Content/Common/Gateway/CriterionHandler/ContentTypeIdentifier.php Show resolved Hide resolved

andrerom approved these changes Jan 8, 2020

View reviewed changes

adamwojs approved these changes Jan 14, 2020

View reviewed changes

emodric force-pushed the patch-21 branch from c6171eb to 5f402a9 Compare January 15, 2020 13:35

adamwojs added Improvement Ready for QA labels Jan 16, 2020

alongosz changed the title ~~EZP-31127: Do not use non-existing content types in ContentTypeIdentifier criterion~~ EZP-31127: Skipped non-existing Content Types in ContentTypeIdentifier criterion Jan 16, 2020

alongosz approved these changes Jan 16, 2020

View reviewed changes

emodric force-pushed the patch-21 branch from bedbe7f to 0ef9b7f Compare January 16, 2020 12:32

micszo self-assigned this Jan 22, 2020

emodric added 4 commits January 23, 2020 10:10

Do not use non-existing content types in ContentTypeIdentifier criterion

92b58c2

Use proper exception, fix import

5aee053

Return an expression matching nothing if there are no valid IDs

f625178

Add test cases for ContentTypeIdentifier criterion with invalid conte…

d8e03f0

…nt types

Log invalid content type identifiers with a warning

f8f16e2

emodric force-pushed the patch-21 branch from 0ef9b7f to f8f16e2 Compare January 23, 2020 09:10

micszo approved these changes Jan 31, 2020

View reviewed changes

micszo added QA approved and removed Ready for QA labels Jan 31, 2020

micszo removed their assignment Jan 31, 2020

lserwatka merged commit d9f9a21 into ezsystems:7.5 Jan 31, 2020

emodric added a commit to netgen-layouts/layouts-ezplatform-relation-list-query that referenced this pull request Mar 4, 2020

Use ContentTypeIdentifier criterion directly

4936035

Fixed in eZ Publish kernel 7.5.7 ezsystems/ezpublish-kernel#2872

emodric added a commit to netgen-layouts/layouts-ezplatform-tags-query that referenced this pull request Mar 4, 2020

Use ContentTypeIdentifier criterion directly, Use $content->getConten…

f8f48a9

…tType() directly Fixed in eZ Publish kernel 7.5.7 ezsystems/ezpublish-kernel#2872

emodric added a commit to netgen-layouts/content-browser-ezplatform that referenced this pull request Mar 4, 2020

Replace usage of ContentTypeId criterion with ContentTypeIdentifier

4597e2d

Related issue fixed in eZ Publish kernel 7.5.7 ezsystems/ezpublish-kernel#2872

emodric added a commit to netgen-layouts/layouts-ezplatform-relation-list-query that referenced this pull request Apr 14, 2020

Use ContentTypeIdentifier criterion directly

9a9b6f9

Fixed in eZ Publish kernel 7.5.7 ezsystems/ezpublish-kernel#2872

emodric added a commit to netgen-layouts/layouts-ezplatform-relation-list-query that referenced this pull request Apr 14, 2020

Use ContentTypeIdentifier criterion directly

0aaae66

Fixed in eZ Publish kernel 7.5.7 ezsystems/ezpublish-kernel#2872

emodric added a commit to netgen-layouts/layouts-ezplatform-relation-list-query that referenced this pull request Apr 14, 2020

Use ContentTypeIdentifier criterion directly

73e8217

Fixed in eZ Publish kernel 7.5.7 ezsystems/ezpublish-kernel#2872

emodric added a commit to netgen-layouts/layouts-ezplatform-relation-list-query that referenced this pull request Jul 9, 2020

Use ContentTypeIdentifier criterion directly

db182fe

Fixed in eZ Publish kernel 7.5.7 ezsystems/ezpublish-kernel#2872

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EZP-31127: Skipped non-existing Content Types in ContentTypeIdentifier criterion #2872

EZP-31127: Skipped non-existing Content Types in ContentTypeIdentifier criterion #2872

emodric commented Nov 14, 2019 •

edited

Loading

emodric commented Dec 13, 2019

emodric commented Jan 8, 2020

andrerom commented Jan 8, 2020

emodric commented Jan 14, 2020

andrerom commented Jan 15, 2020

emodric commented Jan 15, 2020

emodric commented Jan 15, 2020

andrerom commented Jan 15, 2020 •

edited

Loading

emodric commented Jan 15, 2020

emodric commented Jan 15, 2020 •

edited by andrerom

Loading

alongosz Jan 16, 2020

emodric Jan 16, 2020

andrerom Jan 16, 2020

emodric Jan 16, 2020 •

edited

Loading

emodric commented Jan 22, 2020

micszo commented Jan 22, 2020

emodric commented Jan 22, 2020

micszo commented Jan 23, 2020

emodric commented Jan 23, 2020

micszo left a comment

lserwatka commented Jan 31, 2020

alongosz commented Jan 31, 2020

emodric commented Jan 31, 2020

	// Skip non-existing content types
	// Skip non-existing content types, but track for code below
	$invalidIdentifiers[] = $identifier;

EZP-31127: Skipped non-existing Content Types in ContentTypeIdentifier criterion #2872

EZP-31127: Skipped non-existing Content Types in ContentTypeIdentifier criterion #2872

Conversation

emodric commented Nov 14, 2019 • edited Loading

emodric commented Dec 13, 2019

emodric commented Jan 8, 2020

andrerom commented Jan 8, 2020

emodric commented Jan 14, 2020

andrerom commented Jan 15, 2020

emodric commented Jan 15, 2020

emodric commented Jan 15, 2020

andrerom commented Jan 15, 2020 • edited Loading

emodric commented Jan 15, 2020

emodric commented Jan 15, 2020 • edited by andrerom Loading

alongosz Jan 16, 2020

Choose a reason for hiding this comment

emodric Jan 16, 2020

Choose a reason for hiding this comment

andrerom Jan 16, 2020

Choose a reason for hiding this comment

emodric Jan 16, 2020 • edited Loading

Choose a reason for hiding this comment

emodric commented Jan 22, 2020

micszo commented Jan 22, 2020

emodric commented Jan 22, 2020

micszo commented Jan 23, 2020

emodric commented Jan 23, 2020

micszo left a comment

Choose a reason for hiding this comment

lserwatka commented Jan 31, 2020

alongosz commented Jan 31, 2020

emodric commented Jan 31, 2020

emodric commented Nov 14, 2019 •

edited

Loading

andrerom commented Jan 15, 2020 •

edited

Loading

emodric commented Jan 15, 2020 •

edited by andrerom

Loading

emodric Jan 16, 2020 •

edited

Loading