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.

Sign up for GitHub

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

Jump to bottom

EZP-31103: Introduced strict types for ContentTypeService #2856

Merged
merged 1 commit into from
Nov 15, 2019
Merged

EZP-31103: Introduced strict types for ContentTypeService #2856

merged 1 commit into from
Nov 15, 2019

Conversation

adamwojs
Copy link
Member

@adamwojs adamwojs commented Nov 4, 2019 •
edited
Loading

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

This PR introduces strict types for the \eZ\Publish\API\Repository\ContentTypeService

TODO:

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

@adamwojs adamwojs self-assigned this Nov 4, 2019
mikadamczyk
mikadamczyk reviewed Nov 6, 2019
View reviewed changes
Copy link
Contributor

@mikadamczyk mikadamczyk left a comment •
edited
Loading

Choose a reason for hiding this comment

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

CI is failing here

eZ/Publish/Core/MVC/Symfony/Templating/Tests/Twig/Extension/ContentExtensionTest.php Show resolved Hide resolved
eZ/Publish/Core/MVC/Symfony/Templating/Tests/Twig/Extension/ContentExtensionTest.php Outdated Show resolved Hide resolved
eZ/Publish/Core/Persistence/Legacy/Content/Type/Gateway/DoctrineDatabase.php Show resolved Hide resolved
eZ/Publish/Core/Repository/ContentTypeService.php Outdated Show resolved Hide resolved
eZ/Publish/Core/Repository/ContentTypeService.php Outdated Show resolved Hide resolved
eZ/Publish/Core/Repository/ContentTypeService.php Show resolved Hide resolved
eZ/Publish/Core/Repository/ContentTypeService.php Outdated Show resolved Hide resolved
eZ/Publish/SPI/Repository/Tests/Decorator/ContentTypeServiceDecoratorTest.php Outdated Show resolved Hide resolved
eZ/Publish/SPI/Repository/Tests/Decorator/ContentTypeServiceDecoratorTest.php Outdated Show resolved Hide resolved
@adamwojs adamwojs force-pushed the content_type_service_typehint branch from 72ed141 to 15bd08e Compare November 7, 2019 11:06
@adamwojs
Copy link
Member Author

adamwojs commented Nov 7, 2019

PR rebased and updated according to @mikadamczyk code review suggestions.

@adamwojs adamwojs requested review from alongosz and ViniTou November 7, 2019 11:29
ViniTou
ViniTou reviewed Nov 7, 2019
View reviewed changes
Copy link
Contributor

@ViniTou ViniTou left a comment •
edited
Loading

Choose a reason for hiding this comment

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

As there is no way to mark this up in PR.

we have

    /**
     * Bulk-load Content Type objects by ids.
     * @param mixed[] $contentTypeIds
     */
    public function loadContentTypeList(array $contentTypeIds, array $prioritizedLanguages = []): iterable;

maybe we should also improve typehints in docs, so in that case made it * @param int[] $contentTypeIds

@adamwojs
Copy link
Member Author

adamwojs commented Nov 8, 2019

PR updated according to @mikadamczyk @ViniTou code review suggestions.

alongosz
alongosz approved these changes Nov 13, 2019
View reviewed changes
eZ/Publish/Core/Persistence/Legacy/Content/Type/Gateway/DoctrineDatabase.php Show resolved Hide resolved
eZ/Publish/Core/Persistence/Legacy/Content/Type/Gateway/DoctrineDatabase.php
@@ -353,7 +353,7 @@ public function insertType(Type $type, $typeId = null)
$q->prepare()->execute();

if (empty($typeId)) {
$typeId = $this->dbHandler->lastInsertId(
$typeId = (int)$this->dbHandler->lastInsertId(
Copy link
Member

Choose a reason for hiding this comment

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

this one also for 7.5

eZ/Publish/Core/Persistence/Legacy/Content/Type/Gateway/DoctrineDatabase.php
@@ -130,7 +130,7 @@ public function __construct(DatabaseHandler $db, Connection $connection, MaskGen
*
* @param \eZ\Publish\SPI\Persistence\Content\Type\Group $group
*
* @return mixed Group ID
* @return int Group ID
*/
public function insertGroup(Group $group)
Copy link
Member

Choose a reason for hiding this comment

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

Anything stopping us now from adding return type?

Suggested change
public function insertGroup(Group $group)
public function insertGroup(Group $group): int

@micszo micszo self-assigned this Nov 14, 2019
@micszo
Copy link
Member

micszo commented Nov 14, 2019

Testing discovered: https://jira.ez.no/browse/EZP-31131

micszo
micszo requested changes Nov 15, 2019
View reviewed changes
Copy link
Member

@micszo micszo left a comment

Choose a reason for hiding this comment

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

Editing a Content Type form the right side menu causes exception:
Argument 1 passed to eZ\Publish\SPI\Repository\Decorator\ContentTypeServiceDecorator::loadContentTypeGroup() must be of the type int, string given, called in /Users/michalszoltysek/Projects/workspace/ez4.lh/vendor/ezsystems/ezplatform-admin-ui/src/lib/Form/DataTransformer/ContentTypeGroupTransformer.php on line 40
(editing with the "orange pencil" works fine)
Screenshot 2019-11-15 at 10 58 17

@adamwojs adamwojs mentioned this pull request Nov 15, 2019
Merged
2 tasks
@adamwojs
Copy link
Member Author

FYI: The issue mentioned above has been fixed in ezsystems/ezplatform-admin-ui#1133

@micszo
Copy link
Member

micszo commented Nov 15, 2019

Testing discovered: https://jira.ez.no/browse/EZEE-2930

micszo
micszo approved these changes Nov 15, 2019
View reviewed changes
Copy link
Member

@micszo micszo left a comment

Choose a reason for hiding this comment

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

Regression suite OK.
Sanities OK.
Above issue is fixed.

@micszo micszo removed their assignment Nov 15, 2019
@adamwojs adamwojs force-pushed the content_type_service_typehint branch from e0081b0 to c6b76e2 Compare November 15, 2019 14:29
@adamwojs adamwojs merged commit a050cbc into master Nov 15, 2019
@adamwojs adamwojs deleted the content_type_service_typehint branch November 15, 2019 14:29
@adamwojs adamwojs mentioned this pull request Dec 20, 2019
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ViniTou ViniTou ViniTou approved these changes

@micszo micszo micszo approved these changes

@alongosz alongosz alongosz approved these changes

@mikadamczyk mikadamczyk mikadamczyk approved these changes

Assignees

@adamwojs adamwojs

Labels
DX Improvement QA approved
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@adamwojs @micszo @mikadamczyk @alongosz @ViniTou