Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
Now that TranslationManager::translate()
no longer translates, but rather returns
a TranslationWrapper we could simplify the call chain of t()
and $this->t()
quite a lot.
Proposed resolution
convert t()
and $this->t()
to return a TranslationWrapper
Remaining tasks
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#68 | 2600118-68.patch | 13.79 KB | stefan.r |
#68 | interdiff.txt | 642 bytes | stefan.r |
#66 | 2600118-66.patch | 13.74 KB | stefan.r |
#66 | interdiff.txt | 670 bytes | stefan.r |
#65 | 2600118-65.patch | 13.93 KB | stefan.r |
Comments
Comment #2
sdstyles CreditAttribution: sdstyles at FFW commentedComment #4
dawehnerThis should ideally set the translation translation manager on the newly created object, something like
$translatable_markup = new TranslatableMarkup($string, $args, $options, $this->getStringTranslation())
Comment #5
sdstyles CreditAttribution: sdstyles at FFW commented@dawehner thanks for the hint
Comment #7
ramirez.gerardo CreditAttribution: ramirez.gerardo as a volunteer commentedPer @dawehner comments. Patch should reflect code provided in comment 4.
Comment #8
dawehner#5 is IMHO the right patch but it maybe needs some proper fixes inside those tests ...
Comment #9
anil280988 CreditAttribution: anil280988 at Publicis Sapient for Publicis Sapient commentedHi Dawehner,
Could you drop some hint regarding this.
Comment #12
dawehnerSo a) certainly start with #5, #7 is wrong.
Let's have a look at one of those failure:
there you see the following code:
instead use something like:
I kind of believe that a similar approach can be applied to all of those instances.
Comment #13
aerozeppelin CreditAttribution: aerozeppelin commentedBased on comment #12. Here are the some of the changes.
Comment #14
aerozeppelin CreditAttribution: aerozeppelin commentedComment #15
aerozeppelin CreditAttribution: aerozeppelin commentedComment #16
aerozeppelin CreditAttribution: aerozeppelin commentedComment #21
dawehnerThank you for trying to get the patch green!
Some points
a) We don't need to change / convert $this->t() to new TranslationWrapper
b) In order to fix the tests we need to change two things:
1) \Drupal\Tests\Core\Controller\TitleResolverTest::testStaticTitleWithParameter should no longer expects call to $this->translationManager->translate ... so just remove that blob
2) The expected values in \Drupal\Tests\Core\Controller\TitleResolverTest::providerTestStaticTitleWithParameter are now TranslationWrapper objects, so set them up like:
3) We can remove the 'static title !test' testcase in that function, its no longer valid.
Comment #22
anil280988 CreditAttribution: anil280988 at Publicis Sapient for Publicis Sapient commentedCreated the patch
Comment #23
anil280988 CreditAttribution: anil280988 at Publicis Sapient for Publicis Sapient commentedChanging the status
Comment #28
aerozeppelin CreditAttribution: aerozeppelin commentedThanks @dawehner for the hints.
In reference to #21, made changes to Drupal\Tests\Core\Controller\TitleResolverTest::testStaticTitleWithParameter. Point 'a' needs additional work.
Comment #29
anil280988 CreditAttribution: anil280988 at Publicis Sapient for Publicis Sapient commentedFor the 'a' point , as stated by @dawehner , $this->t() need not be changed to new TranslationWrapper. So changing those line back.
Comment #31
dawehner@anil280988
Did you tried to create a patch file by hand?
Comment #32
aerozeppelin CreditAttribution: aerozeppelin commentedWith reference to #21. Made some changes to accomplish this.
Comment #33
dawehnerNone of these changes are needed, these php arrays are optional, see
StringTranslationTrait::t()
Unneeded
Comment #34
anil280988 CreditAttribution: anil280988 at Publicis Sapient for Publicis Sapient commentedRolled back the changes listed @dawehner.
Comment #35
dawehnerthis is now also not needed anymore
Comment #36
anil280988 CreditAttribution: anil280988 at Publicis Sapient for Publicis Sapient commentedDone Changes.
Comment #37
dawehnerThank you!
Comment #45
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commented@Anil thanks for the patch. Making it again to RTBC as it is working fine for me. Don't know what happened to testbot.
That is the output for Applying patch.
Comment #51
alexpottLet's do format_plural and formatPlural whilst we are at it to keep things consistent.
Comment #52
priya.chat CreditAttribution: priya.chat at Publicis Sapient for Publicis Sapient commented@Alexpott, your suggestion/ comment is not clear to me. can you please explain a bit more.
Comment #53
dawehnerIf you look at
\Drupal\Core\StringTranslation\StringTranslationTrait::formatPlural
you'll see that it calls to\Drupal\Core\StringTranslation\TranslationInterface::formatPlural
which is just returning a new object, exactly the same as t() does on that class.Comment #54
dawehnerUnassigning and keeping the novice task to see whether someone else might pick up the issue.
Comment #55
dawehnerthis function no longer exists.
Fixed that and also adapted the test coverage to be a bit better.
Comment #57
dawehnerThere we go.
Comment #58
dawehnerIs this still valuable for 8.0.x as well, I would say so.
Comment #59
Crell CreditAttribution: Crell as a volunteer commentedIf the bot's OK with it, this is RTBC. If the bot's not still OK with it, it can set it back.
Comment #60
catchDon't see what we get from this in 8.0.x, and it could potentially cause contrib test failures. But looks good to me for 8.1.x - moving there for the bot.
Comment #62
dawehnerWell, things are easier to understand. Previously its not obviously that t() is just a wrapper for the TranslatableMarkup object, which is kinda the point of the issue.
Comment #63
catchWell looks like it needs a re-roll for 8.1.x at least.
Comment #64
andypostre-roll against 8.1.x
removed in re-roll cos unused
Comment #65
stefan.r CreditAttribution: stefan.r commentedNice spot!
Wrong comment got removed during that re-roll :)
Comment #66
stefan.r CreditAttribution: stefan.r commentedComment #67
andyposts/translate/translateString
Comment #68
stefan.r CreditAttribution: stefan.r commentedComment #69
andypostComment #71
catchCommitted/pushed to 8.1.x, thanks!
Leaving RTBC against 8.0.x.
Again I struggle with this when there's just two months until 8.0.x EOL.
Comment #72
alexpottI agree with @catch here - it's not fixing a bug and the disruption to 8.0.x contrib does not seem worth it. Given that credit for working on issues is not given whilst it remains open I'm going to mark the issue as fixed. Also
t()
documents that it returns aTranslatableMarkup
object so I'm not that convinced that the win is big enough for 8.0.x.Comment #73
dawehnerFair, I always forget that time moves on.
Comment #74
tstoeckler