Implement support for content translation

Just came across this issue again. @SpadXIII and the others here: Are you using the patch successfully?
We should try to get this fixed and review it asap. Could you provide feedback and help to finish this? (I'm going to do the same)

I found several issues with the code and the tests, I'm moving this to needs work as I'm currently refactoring the tests.

I fixed the problems I found and refactored the test to include the translation testing with the original feature tests.

I'm uploading a patch file for composer patching.

The last submitted patch, 19: 3119626-19.patch, failed testing. View results →

Thank you very very much @joevagyok, this looks excellent. Still the patch is quite hard to review, due to the several changes.

Settings this back to NW for two reasons:
1. Some code style issues were newly introduced
2. We should have further tests for the different cases, with and without translation, to test the different conditions. Best would be to keep the existing test unchanged and add a separate test class for translation testing. Existing test should work, if translation is not enabled. The other test class(es) should check the more complex cases.

I tend to disagree with the original test. The original test was incorrect and inefficient, asserting things unrelated to the feature, and missing tests for several parts of the module, leaving actual bugs in place, and using non-verbose assertions making it difficult to understand what's going on. Having translations in the same test case or not, doesn't really matter code wise, but if the wish is the separate the two I could make that happen.

@joevagyok I didn't mean to criticize your implementation. Just meant to say, that still important tests are missing and we should use this chance to have a test with and one without translation enabled (as separate test classes so that we can test with and without the l10n modules.
That way we can be sure the module (still) works as expected and improve the stability with and without translation. Do you agree?

Of course it's not "your" task to do that, but I'm sure we'll all be thankful for the one who does :)

Or in other words: If it's not done now, it won't be done and that's bad. This is a good chance to test both cases now you're in the code logic.

Absolutely, I agree. It seems I misunderstood your intentions with the original tests.

Will have more time to review this next week. Thanks for all the work - I don't personally maintain multilingual sites but I understand it's very important to add this compatibility

I simplified the logic in the patch and did some cleaned up.

A note: in cases when the user could override the default language media file (ex. EN if the file was created in EN) from a translation, we don't render the replacement widget. For that case, I might add a little message for the user informing the file replacement override is not possible because the current file on the translation is the one from the default language version. Welcome any advice on that from UX standpoint.
Once the user uploads a different file from that using the original widget, file replacement is allowed, because we don't risk to change the content of the file across translations.

@joevagyok thank you very much! Did you already consider / check the cases from #8?

Do you know if we have tests for all the relevant combinations already?

Cases from #8 rely on the assumption that the replacement field should be visible without its override checkbox, which is counterintuitive in my opinion.
The rendering of the replacement widget without the checkbox of override, is kind of pointless, since users can use the default file widget to "Remove" and "Upload" a new file which is a default behaviour well known for users, and it is exactly the same behaviour of "Replacement field" without checkbox. Therefore, having behaviours like, showing replacement widget without override checkbox and showing it with checkbox, is just inconsistent, the end user has no idea why a checkbox goes away. Having cases which supports replacement in its full functionality or not, is rather clear.

The changes look good to me! As we have too many changes I would wait for other reviews.

Test coverage is good and the functionality works for multilingual sites as expected. At this stage I would move the issue to RTBC.

Can the issue summary please be updated to properly describe the issue and the solution? I can adapt that to the release notes after committing this

Huge thanks to everyone that worked on this issue. I've set a reminder to put out a new release containing this code in 2 weeks. If the EC team is using this on their production sites and find any issues, I'd rather those issues surface before cutting a new release.

Automatically closed - issue fixed for 2 weeks with no activity.

Adding patch file for composer patching.

I've released version 8.x-1.2 that includes this issue.