Skip to content

Add methods used by TranslationUpdateCommand #25860

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

Closed
wants to merge 2 commits into from
Closed

Add methods used by TranslationUpdateCommand #25860

wants to merge 2 commits into from

Conversation

ebuildy
Copy link
Contributor

@ebuildy ebuildy commented Jan 20, 2018

Symfony\Bundle\FrameworkBundle\Command\TranslationUpdateCommand uses TranslationWriterInterface methods disableBackup and getFormats not defined by the interface but by the implementation Symfony\Component\Translation\Writer\TranslationWriter.

Q A
Branch? master
Bug fix? no
New feature? no
BC breaks? yes (
Deprecations? no
Tests pass? yes/no
Fixed tickets #...
License MIT

``Symfony\Bundle\FrameworkBundle\Command\TranslationUpdateCommand`` uses ``TranslationWriterInterface`` methods ``disableBackup`` and ``getFormats`` not defined by the interface but by the implementation ``Symfony\Component\Translation\Writer\TranslationWriter``.
@linaori
Copy link
Contributor

linaori commented Jan 20, 2018

If you want to make it backwards compatible, you'll probably have to make a second interface and implement that. BC breaks won't me merged until 5.0. In your PR, this is also seen as a new feature.

@chalasr chalasr added this to the 4.1 milestone Jan 20, 2018
@ebuildy
Copy link
Contributor Author

ebuildy commented Jan 22, 2018

I dont want to add more complexity by adding a new interface, can wait for v5.0 since Php is not Java ^^.

Also, there is the same problem with: $kernel = $this->getApplication()->getKernel();: $this->application should be a Symfony\Component\Console\Application as defined in Command parent class. getKernel is present on Symfony\Bundle\FrameworkBundle\Console\Application, a sub-class of Symfony\Component\Console\Application.

@linaori
Copy link
Contributor

linaori commented Jan 22, 2018

I dont want to add more complexity by adding a new interface, can wait for v5.0 since Php is not Java

One of the 4.x versions will have to add a layer to enable this feature either way. V5 won't be released for almost 2 years ;)

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.

We don't break BC in major releases without an upgrade path in a previous minor release.
If one comes with a valid use case for decorating the translation.writer service, we could add an instanceof check to skip calling the getFormats() method. The same goes for the calls to Application::getKernel(), yet it's not super clean but that's under framework-bundle, we know which implementation is behind it.
👎 Fine as is to me

/**
* Disables dumper backup.
*/
public function disableBackup();
Copy link
Member

Choose a reason for hiding this comment

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

doesn't have to be interfaced:

* @deprecated since Symfony 4.1, to be removed in 5.0

@chalasr
Copy link
Member

chalasr commented Feb 3, 2018

Closing as explained, thanks for proposing.

@chalasr chalasr closed this Feb 3, 2018
fabpot added a commit that referenced this pull request Oct 28, 2020
…when using "--no-backup" (liarco)

This PR was squashed before being merged into the 5.1 branch.

Discussion
----------

[FrameworkBundle] Fixing TranslationUpdateCommand failure when using "--no-backup"

| Q             | A
| ------------- | ---
| Branch?       | 5.1
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       |
| License       | MIT
| Doc PR        |

[5.0.0](https://github.com/symfony/symfony/blob/5.0/src/Symfony/Component/Translation/CHANGELOG.md#500) removed `TranslationWriter::disableBackup()` but `TranslationUpdateCommand` still has `--no-backup` flag. Using that flag throws an error so I think that removing it without deprecation may be the right choice.

Thrown error:
```
In TranslationUpdateCommand.php line 287:

  Attempted to call an undefined method named "disableBackup" of class "Symfony\Component\Translation\Writer\TranslationWriter".
```

Further references to the topic:
 - #18290 (comment)
 - #25860

Commits
-------

ef24b10 [FrameworkBundle] Fixing TranslationUpdateCommand failure when using "--no-backup"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants