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

[Translation] [Loco] Send If-Modified-Since header when possible #44484

Merged
merged 0 commits into from
Feb 4, 2022
Merged

[Translation] [Loco] Send If-Modified-Since header when possible #44484

merged 0 commits into from
Feb 4, 2022

Conversation

Kocal
Copy link
Contributor

@Kocal Kocal commented Dec 6, 2021 •
edited
Loading

Q A
Branch? 6.1
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix #44414
License MIT
Doc PR symfony/symfony-docs#...

This PR is a proposal for #44414.

When pulling translations from Loco, we save the Last-Modified response header in each MessageCatalogue thanks to the new CatalogueMetadataAwareInterface.

When pulling translations one more time from Loco, we send the previous Last-Modified response header as If-Modified-Since request header if it's available.
If Loco returns a 304, we don't update current translations. If it's not the case, we keep the original behaviour.

Catalogue metadata can be used to store metadata about the catalogue itself and not about translations.
Actually, only .xliff format is supported (read/write).

WDYT?

Thanks!

cc @welcoMattic

@carsonbot carsonbot added this to the 6.1 milestone Dec 6, 2021
@carsonbot carsonbot changed the title [Translation][Loco] Send \If-Modified-Since\ header when possible, close #44414 [Translation] [Loco] Send \If-Modified-Since\ header when possible, close #44414 Dec 6, 2021
@Kocal Kocal changed the title [Translation] [Loco] Send \If-Modified-Since\ header when possible, close #44414 [Translation] [Loco] Send If-Modified-Since header when possible, close #44414 Dec 6, 2021
Kocal
Kocal commented Dec 6, 2021
View reviewed changes
src/Symfony/Component/Translation/Resources/schemas/xliff-core-1.2-transitional.xsd Outdated
Comment on lines 1845 to 1862
<xsd:element name="prop-group">
<xsd:complexType>
<xsd:sequence maxOccurs="unbounded">
<xsd:element ref="xlf:prop"/>
</xsd:sequence>
<xsd:attribute name="name" type="xsd:string" use="optional"/>
</xsd:complexType>
</xsd:element>
<xsd:element name="prop">
<xsd:complexType>
<xsd:simpleContent>
<xsd:extension base="xsd:string">
<xsd:attribute name="prop-type" type="xsd:string" use="required"/>
<xsd:attribute ref="xml:lang" use="optional"/>
</xsd:extension>
</xsd:simpleContent>
</xsd:complexType>
</xsd:element>
Copy link
Contributor Author

@Kocal Kocal Dec 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I had to add declarations prop-group/prop, otherwise it breaks because we have some validations on XLIFF files.

Taken from https://github.com/sap-archive/xliff-1-2/blob/master/com.sap.mlt.xliff12.impl/src/main/java/com/sap/mlt/xliff12/impl/schema/xliff-core-1.2-transitional.xsd

@Kocal
Copy link
Contributor Author

Kocal commented Dec 7, 2021

I'm not sure to understand why PHPUnit tests are failing:

Symfony\Component\DependencyInjection\Exception\InvalidArgumentException: Invalid definition for service "translation.provider_factory.loco": "Symfony\Component\Translation\Bridge\Loco\LocoProviderFactory::__construct()" requires 5 arguments, 4 passed

The file src/Symfony/Bundle/FrameworkBundle/Resources/config/translation_providers.php has been updated and service reference translator have been added as 5th argument to translation.provider_factory.loco service, but it fails.

Do someknow what's going on here? Thanks!

@carsonbot
Copy link

Hey!

I think @rvanlaak has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

welcoMattic
welcoMattic requested changes Dec 9, 2021
View reviewed changes
Copy link
Member

@welcoMattic welcoMattic 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.

Here are some little comments.
I guess we need to update the changelog of Translation component, because this PR add support of prop-group and prop XLIFF tags?

WDYT @stof @nicolas-grekas?

src/Symfony/Component/Translation/Bridge/Loco/LocoProvider.php Outdated Show resolved Hide resolved
src/Symfony/Component/Translation/Catalogue/TargetOperation.php Outdated Show resolved Hide resolved
src/Symfony/Component/Translation/MessageCatalogue.php Outdated Show resolved Hide resolved
src/Symfony/Component/Translation/MessageCatalogue.php Outdated Show resolved Hide resolved
src/Symfony/Component/Translation/MessageCatalogue.php Outdated Show resolved Hide resolved
src/Symfony/Component/Translation/MessageCatalogue.php Outdated Show resolved Hide resolved
@Kocal
Copy link
Contributor Author

Kocal commented Dec 10, 2021

Thanks for your comments :)

We still have the issue when running tests, and this issue only happens in CI:

Symfony\Component\DependencyInjection\Exception\InvalidArgumentException: Invalid definition for service "translation.provider_factory.loco": "Symfony\Component\Translation\Bridge\Loco\LocoProviderFactory::__construct()" requires 5 arguments, 4 passed

ping @nicolas-grekas @stof, do you know what can happens here? Thanks!

rvanlaak
rvanlaak reviewed Dec 12, 2021
View reviewed changes
Copy link
Contributor

@rvanlaak rvanlaak left a comment

Choose a reason for hiding this comment

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

Looks great already. Do you have an idea whether the other translation providers also support the If-Modified-Since header?

src/Symfony/Component/Translation/CatalogueMetadataAwareInterface.php Outdated Show resolved Hide resolved
src/Symfony/Component/Translation/CatalogueMetadataAwareInterface.php Outdated Show resolved Hide resolved
src/Symfony/Component/Translation/Util/XliffUtils.php Outdated Show resolved Hide resolved
src/Symfony/Component/Translation/Bridge/Loco/CHANGELOG.md Outdated Show resolved Hide resolved
@nicolas-grekas nicolas-grekas changed the title [Translation] [Loco] Send If-Modified-Since header when possible, close #44414 [Translation][Loco] Send If-Modified-Since header when possible Dec 13, 2021
@Kocal
Copy link
Contributor Author

Kocal commented Dec 14, 2021

Status: needs review

@carsonbot carsonbot changed the title [Translation][Loco] Send If-Modified-Since header when possible [Translation] [Loco] Send If-Modified-Since header when possible Dec 14, 2021
welcoMattic
welcoMattic approved these changes Dec 21, 2021
View reviewed changes
Copy link
Member

@welcoMattic welcoMattic left a comment

Choose a reason for hiding this comment

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

Code is ok for me, but we will need @nicolas-grekas or @stof help to fix the failing test, I have difficulties to understand what is wrong here

chalasr
chalasr requested changes Dec 21, 2021
View reviewed changes
Copy link
Member

@chalasr chalasr left a comment

Choose a reason for hiding this comment

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

Adding BC layers for the added constructor parameters should help fixing the failing tests, at least the high-deps one.
For low-deps, we may need to bump the symfony/translation requirement in Loco Bridge's composer.json to ^6.1 to make it always has the new interfaces.

src/Symfony/Component/Translation/CatalogueMetadataAwareInterface.php Outdated Show resolved Hide resolved
src/Symfony/Component/Translation/Bridge/Loco/LocoProvider.php Outdated Show resolved Hide resolved
src/Symfony/Component/Translation/Bridge/Loco/LocoProviderFactory.php Outdated Show resolved Hide resolved
@chalasr
Copy link
Member

chalasr commented Dec 21, 2021

New constructor parameters should be mentioned in CHANGELOG/UPGRADE files also

@Kocal
Copy link
Contributor Author

Kocal commented Dec 21, 2021 •
edited
Loading

Thanks for your comments @chalasr, the symfony/translation dependency bump was the key! :)

The failures are unrelated IMO.

Status: needs review

@Kocal
Copy link
Contributor Author

Kocal commented Dec 28, 2021

Review comments have been addressed, deprecations have been removed and some tests fixed.
Failures are not related.

Status: needs review

nicolas-grekas
nicolas-grekas reviewed Dec 28, 2021
View reviewed changes
...fony/Component/Translation/Bridge/Loco/Tests/LocoProviderFactoryWithoutTranslatorBagTest.php Outdated Show resolved Hide resolved
...fony/Component/Translation/Bridge/Loco/Tests/LocoProviderFactoryWithoutTranslatorBagTest.php Outdated
*/
class LocoProviderFactoryWithoutTranslatorBagTest extends LocoProviderFactoryTest
{
use ExpectDeprecationTrait;
Copy link
Member

Choose a reason for hiding this comment

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

I guess the full test case can be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it would be nice to ensure the LocoProviderFactory still works even without the TranslatorBag. WDYT?

src/Symfony/Component/Translation/Bridge/Loco/Tests/LocoProviderWithoutTranslatorBagTest.php Outdated Show resolved Hide resolved
src/Symfony/Component/Translation/Bridge/Loco/Tests/LocoProviderWithoutTranslatorBagTest.php Outdated Show resolved Hide resolved
src/Symfony/Component/Translation/Dumper/XliffFileDumper.php Outdated Show resolved Hide resolved
src/Symfony/Component/Translation/Dumper/XliffFileDumper.php Outdated Show resolved Hide resolved
src/Symfony/Component/Translation/MessageCatalogue.php Outdated Show resolved Hide resolved
@Kocal
Copy link
Contributor Author

Kocal commented Jan 5, 2022

The PR has been rebased, failures are not related.

@Kocal
Copy link
Contributor Author

Kocal commented Feb 4, 2022

Is there anything else to do?

@fabpot
Copy link
Member

fabpot commented Feb 4, 2022

Thank you @Kocal.

@fabpot fabpot closed this Feb 4, 2022
@fabpot fabpot merged commit 74b8d16 into symfony:6.1 Feb 4, 2022
@Kocal Kocal deleted the feat/GH-44414-translation-loco-send-if-modified-since branch February 4, 2022 10:17
@fabpot fabpot mentioned this pull request Apr 15, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@derrabus derrabus derrabus left review comments

@rvanlaak rvanlaak rvanlaak left review comments

@fabpot fabpot fabpot approved these changes

@welcoMattic welcoMattic welcoMattic approved these changes

@chalasr chalasr chalasr approved these changes

@nicolas-grekas nicolas-grekas Awaiting requested review from nicolas-grekas

Assignees
No one assigned
Labels
Feature Status: Reviewed Translation
Projects
None yet
Milestone
6.1
Development

Successfully merging this pull request may close these issues.

[Translation][Loco] Consider sending If-Modified-Since or If-None-Match header when pulling translations
8 participants
@Kocal @carsonbot @chalasr @fabpot @nicolas-grekas @welcoMattic @derrabus @rvanlaak