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-30646: Reimplemented SignalSlots using EventDispatcher #91

Merged
merged 4 commits into from
Aug 5, 2019
Merged

EZP-30646: Reimplemented SignalSlots using EventDispatcher #91

merged 4 commits into from
Aug 5, 2019

Conversation

Nattfarinn
Copy link
Contributor

@Nattfarinn Nattfarinn commented Jul 5, 2019 •
edited by alongosz
Loading

Question Answer
JIRA issue EZP-30646
Type Feature
Target version master
BC breaks yes
Doc needed no

Depends on: ezsystems/ezpublish-kernel#2682

Tests are still missing, but it has to pass QA ASAP

TODO:

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

@Nattfarinn Nattfarinn mentioned this pull request Jul 5, 2019
Merged
6 tasks
@Nattfarinn Nattfarinn marked this pull request as ready for review July 5, 2019 11:55
ViniTou
ViniTou approved these changes Jul 8, 2019
View reviewed changes
Copy link
Contributor

@ViniTou ViniTou left a comment

Choose a reason for hiding this comment

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

Looks good for me aside of failing CS and one functional test, but is it possible that it is related to removing deprecated \eZ\Publish\Core\SignalSlot\Signal\ContentService\RemoveTranslationSignal?

src/EventSubscriber/CachePurge/AbstractSubscriber.php Outdated Show resolved Hide resolved
src/EventSubscriber/CachePurge/AbstractSubscriber.php Outdated Show resolved Hide resolved
@alongosz alongosz changed the title EZP-30646: SignalSlots reimplemented using EventDispatcher EZP-30646: Reimplemented SignalSlots using EventDispatcher Jul 10, 2019
alongosz
alongosz reviewed Jul 10, 2019
View reviewed changes
Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

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

CS is not correct in all or most of the files.

src/EventSubscriber/CachePurge/AbstractSubscriber.php Show resolved Hide resolved
src/EventSubscriber/CachePurge/AbstractSubscriber.php Show resolved Hide resolved
src/EventSubscriber/CachePurge/AbstractSubscriber.php Outdated Show resolved Hide resolved
src/EventSubscriber/CachePurge/AbstractSubscriber.php Outdated Show resolved Hide resolved
alongosz and others added 2 commits July 17, 2019 16:11
@alongosz
Casted mixed Location and Content Id properties to int
199bebd 
API allows id properties to be string, so they need to be casted.
@Nattfarinn
!fixup
8820d7b
@lserwatka lserwatka merged commit 416a15f into master Aug 5, 2019
@lserwatka lserwatka deleted the ezp-30646 branch August 5, 2019 12:50
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

@mnocon mnocon mnocon approved these changes

@alongosz alongosz alongosz approved these changes

@adamwojs adamwojs Awaiting requested review from adamwojs

@andrerom andrerom Awaiting requested review from andrerom

@webhdx webhdx Awaiting requested review from webhdx

Assignees
No one assigned
Labels
Feature QA approved
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Nattfarinn @alongosz @mnocon @ViniTou @lserwatka