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

Add AdminExtractor #6040

Merged
merged 1 commit into from
Jul 2, 2020
Merged

Conversation

franmomu
Copy link
Member

Subject

The current AdminExtractor is using JMSTranslationBundle. I added a new AdminExtractor that uses translation.extractor Symfony tag, so there is no need of the JMS bundle dependency.

The class is a copy of the other one with some changes. The old one implements TranslatorInterface so it's injected to the admin instances and when AdminInterface::trans is called, it registers it into the catalogue. Since AdminInterface::trans is deprecated, the new extractor does not look into these calls anymore, just to the ones made by the LabelTranslatorStrategy.

I am targeting this branch, because this is BC.

Changelog

### Added
- Added new `AdminExtractor` to extract translations from the Admin classes
### Deprecated
- `AdminExtractor` class for JMSTranslationBundle integration

@franmomu franmomu added the minor label Apr 11, 2020
@franmomu franmomu force-pushed the add_symfony_extractor branch 2 times, most recently from 6249612 to 15e199a Compare April 11, 2020 12:49
@greg0ire
Copy link
Contributor

Please have a look at the FlintCI failures

@franmomu
Copy link
Member Author

I removed the logger since I think it's not useful here.

The FlintCI failures are #6039.

tests/Translator/Extractor/AdminExtractorTest.php Outdated Show resolved Hide resolved
tests/Translator/Extractor/AdminExtractorTest.php Outdated Show resolved Hide resolved
tests/Translator/Extractor/AdminExtractorTest.php Outdated Show resolved Hide resolved
@franmomu franmomu force-pushed the add_symfony_extractor branch from 25f7f51 to e6839f0 Compare April 11, 2020 17:39
@franmomu franmomu force-pushed the add_symfony_extractor branch 2 times, most recently from 6d60490 to d2788a3 Compare April 11, 2020 19:15
phansys
phansys previously approved these changes Apr 11, 2020
}
}

public function setPrefix($prefix): void
Copy link
Member

Choose a reason for hiding this comment

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

Can we add string typehint here?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is from ExtractorInterface.

src/Translator/Extractor/AdminExtractor.php Outdated Show resolved Hide resolved
src/Translator/Extractor/AdminExtractor.php Outdated Show resolved Hide resolved
$this->prefix = $prefix;
}

public function getLabel($label, $context = '', $type = ''): string
Copy link
Member

Choose a reason for hiding this comment

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

Typehints?

Copy link
Member Author

Choose a reason for hiding this comment

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

@franmomu franmomu force-pushed the add_symfony_extractor branch from d2788a3 to 664ff13 Compare April 13, 2020 13:53
@franmomu franmomu requested a review from a team April 13, 2020 15:44
greg0ire
greg0ire previously approved these changes Apr 13, 2020
src/Resources/config/core.xml Outdated Show resolved Hide resolved
@franmomu franmomu force-pushed the add_symfony_extractor branch 2 times, most recently from 19b7485 to f68066d Compare April 16, 2020 22:18
VincentLanglet
VincentLanglet previously approved these changes Apr 16, 2020
@franmomu franmomu force-pushed the add_symfony_extractor branch from f68066d to 728bec7 Compare April 17, 2020 06:29
@VincentLanglet
Copy link
Member

@core23 @phansys Is it ok for you now ?

@VincentLanglet VincentLanglet requested review from core23 and phansys June 30, 2020 23:45
phansys
phansys previously approved these changes Jul 1, 2020
Copy link
Member

@phansys phansys left a comment

Choose a reason for hiding this comment

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

Regardless the small comments I've left, I think it is RTM.

tests/Translator/Extractor/AdminExtractorTest.php Outdated Show resolved Hide resolved
tests/Translator/Extractor/AdminExtractorTest.php Outdated Show resolved Hide resolved
@jordisala1991 jordisala1991 merged commit 555c817 into sonata-project:3.x Jul 2, 2020
@jordisala1991
Copy link
Member

Thank you @franmomu

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants