Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
fabpot merged 0 commits intosymfony:6.1fromKocal:feat/GH-44414-translation-loco-send-if-modified-since
Feb 4, 2022
Merged

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

fabpot merged 0 commits intosymfony:6.1fromKocal:feat/GH-44414-translation-loco-send-if-modified-since
Feb 4, 2022

Conversation

Kocal
Copy link
Member

@KocalKocal commentedDec 6, 2021•
edited
Loading

QA
Branch?6.1
Bug fix?no
New feature?yes
Deprecations?no
TicketsFix#44414
LicenseMIT
Doc PRsymfony/symfony-docs#...

This PR is a proposal for#44414.

When pulling translations from Loco, we save theLast-Modified response header in eachMessageCatalogue thanks to the newCatalogueMetadataAwareInterface.

When pulling translations one more time from Loco, we send the previousLast-Modified response header asIf-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

@carsonbotcarsonbot added this to the6.1 milestoneDec 6, 2021
@carsonbotcarsonbot changed the title[Translation][Loco] Send \If-Modified-Since\ header when possible, close #44414[Translation] [Loco] Send \If-Modified-Since\ header when possible, close #44414Dec 6, 2021
@KocalKocal changed the title[Translation] [Loco] Send \If-Modified-Since\ header when possible, close #44414[Translation] [Loco] SendIf-Modified-Since header when possible, close #44414Dec 6, 2021
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
MemberAuthor

@KocalKocalDec 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 declarationsprop-group/prop, otherwise it breaks because we have some validations on XLIFF files.

Taken fromhttps://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
MemberAuthor

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 filesrc/Symfony/Bundle/FrameworkBundle/Resources/config/translation_providers.php has been updated and service referencetranslator have been added as 5th argument totranslation.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

rvanlaak reacted with hooray emoji

Copy link
Member

@welcoMatticwelcoMattic 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 ofprop-group andprop XLIFF tags?

WDYT@stof@nicolas-grekas?

@Kocal
Copy link
MemberAuthor

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!

Copy link
Contributor

@rvanlaakrvanlaak 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 theIf-Modified-Since header?

@nicolas-grekasnicolas-grekas changed the title[Translation] [Loco] SendIf-Modified-Since header when possible, close #44414[Translation][Loco] SendIf-Modified-Since header when possibleDec 13, 2021
@Kocal
Copy link
MemberAuthor

Status: needs review

@carsonbotcarsonbot changed the title[Translation][Loco] SendIf-Modified-Since header when possible[Translation] [Loco] SendIf-Modified-Since header when possibleDec 14, 2021
Copy link
Member

@welcoMatticwelcoMattic 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

Copy link
Member

@chalasrchalasr 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 thesymfony/translation requirement in Loco Bridge's composer.json to^6.1 to make it always has the new interfaces.

@chalasr
Copy link
Member

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

@Kocal
Copy link
MemberAuthor

Kocal commentedDec 21, 2021•
edited
Loading

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

The failures are unrelated IMO.

Status: needs review

@Kocal
Copy link
MemberAuthor

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

Status: needs review

*/
class LocoProviderFactoryWithoutTranslatorBagTest extends LocoProviderFactoryTest
{
use ExpectDeprecationTrait;

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
MemberAuthor

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 theLocoProviderFactory still works even without theTranslatorBag. WDYT?

@Kocal
Copy link
MemberAuthor

The PR has been rebased, failures are not related.

@Kocal
Copy link
MemberAuthor

Is there anything else to do?

@fabpot
Copy link
Member

Thank you@Kocal.

@fabpotfabpot closed thisFeb 4, 2022
@fabpotfabpot merged commit74b8d16 intosymfony:6.1Feb 4, 2022
@KocalKocal deleted the feat/GH-44414-translation-loco-send-if-modified-since branchFebruary 4, 2022 10:17
@fabpotfabpot mentioned this pull requestApr 15, 2022
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@derrabusderrabusderrabus left review comments

@rvanlaakrvanlaakrvanlaak left review comments

@fabpotfabpotfabpot approved these changes

@welcoMatticwelcoMatticwelcoMattic approved these changes

@chalasrchalasrchalasr approved these changes

@nicolas-grekasnicolas-grekasAwaiting requested review from nicolas-grekas

Assignees
No one assigned
Projects
None yet
Milestone
6.1
Development

Successfully merging this pull request may close these issues.

[Translation][Loco] Consider sendingIf-Modified-Since orIf-None-Match header when pulling translations
8 participants
@Kocal@carsonbot@chalasr@fabpot@nicolas-grekas@welcoMattic@derrabus@rvanlaak
[8]ページ先頭
©2009-2025 Movatter.jp