-
Notifications
You must be signed in to change notification settings - Fork 5
[FEATURE] Add event GlossarySyncDone #17
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
base: main
Are you sure you want to change the base?
[FEATURE] Add event GlossarySyncDone #17
Conversation
sbuerk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @koehnlein for the pull-request.
In general I don't hestitate to accept an event on places if that helps others, but in this case I would abstain from appling and accepting this directly.
The reason is, that my collegue is preparing the deepltranslate-core towards API v3 and I don't have an oversight what changes may required in this area and if the event make sense in the current form. We keep the pull-request open so we can revisit this when we have a deeper insight where the road is going to be layed out.
Additionally, we need to check if a more finer event would also make sense to complete this and further if more context data in the event would be reasonable or use-full.
Beside that, and because it's missing in the pull-request/commit message, what is your exact use-case for what you are requesting this event. That would help us to understand your need and to help out if this is really the right way for your use-case or if we should provide something else.
So please be aware that this may eventually lay around the one or other day (week).
| ClientInterface $client, | ||
| GlossaryRepository $glossaryRepository | ||
| GlossaryRepository $glossaryRepository, | ||
| EventDispatcher $eventDispatcher, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| EventDispatcher $eventDispatcher, | |
| EventDispatcherInterface $eventDispatcher, |
It's common practice to use the EventDispatcherInterface as type declaration and not the concrete implementation. We strive to use interfaces over concrete implementation as much as possible, and refactor the code base over time if not
breaking over time - and if we have the time for it.
For new code we are having a eye on such things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed it
| use DeepL\GlossaryLanguagePair; | ||
| use Doctrine\DBAL\Driver\Exception; | ||
| use TYPO3\CMS\Core\Cache\Frontend\FrontendInterface; | ||
| use TYPO3\CMS\Core\EventDispatcher\EventDispatcher; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| use TYPO3\CMS\Core\EventDispatcher\EventDispatcher; | |
| use Psr\EventDispatcher\EventDispatcherInterface; |
Sorting needs to be checked after applining the change:
Build/Scripts/runTests.sh -s cglThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed it
| */ | ||
| final class GlossarySyncDone | ||
| { | ||
| public function __construct(public readonly int $pageId) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to talk with my collegue, but I guess we are stating with private properties for now and using getter/getter&setters .. needs to be revisited and rethinked.
Coming back on this later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked into other Events of your company's extensions and followed the coding style, I could find there.
- https://github.com/web-vision/deepltranslate-core/blob/7d0694698086337df2eddbc0715e45d6bfb82235/Classes/Event/DeepLGlossaryIdEvent.php#L23
- https://github.com/web-vision/deepltranslate-core/blob/7d0694698086337df2eddbc0715e45d6bfb82235/Classes/Event/DisallowTableFromDeeplTranslateEvent.php#L14
- https://github.com/web-vision/deepltranslate-core/blob/7d0694698086337df2eddbc0715e45d6bfb82235/Classes/Event/RenderLocalizationSelectAllowed.php#L16
But I'm fine if I should change it again.
65bcb65 to
aea8790
Compare
I would like to introduce further processings with glossary entries and set flags, depending if the were change in TYPO3 (I to this with a DataHandler hook) or if they are also synced to DeepL. For this second part, I need this event with the information which page ID was synced. |
First of all, thanks for all of your DeepL extensions, they help me a lot.
It would be pretty nice for my use case to have an event, when the glossary sync was done.