Once #1813762: Introduce unified interfaces, use dependency injection for interface translation lands, the backend for st() and t() will use the same interface (in fact st() just calls t()), so we can get rid of st(), $t() and get_t() for good. marking postponed on that.
Comment | File | Size | Author |
---|---|---|---|
#38 | st-1838310-38.patch | 2.12 KB | Pancho |
#35 | st-1838310-35.patch | 5.62 KB | tim.plunkett |
#29 | drupal-remove_get_t_st-1838310-29.patch | 158.32 KB | ParisLiakos |
#25 | drupal-remove_get_t_st-1838310-25.patch | 158.3 KB | ParisLiakos |
#23 | drupal-remove_get_t_st-1838310-23.patch | 156.88 KB | ParisLiakos |
Comments
Comment #1
Gábor HojtsyComment #2
Gábor HojtsyFix tag.
Comment #3
Jose Reyero CreditAttribution: Jose Reyero commentedThis is mostly the patch, that I'm taking out from the big one in #1813762: Introduce unified interfaces, use dependency injection for interface translation
Comment #4
Gábor HojtsyThis is going to be a tremendous DX improvement! I'm a bit surprised st() is not taken out in this patch. How come?
Comment #5
Jose Reyero CreditAttribution: Jose Reyero commented> I'm a bit surprised st() is not taken out in this patch. How come?
I'm afraid there's still a place for 'st' and this is: Trans
lating installer strings before the container has been created.
Of course, the docs would need to be updated to something like: 'Use st() for strings that may be used by the installer before the container is created.' (Or the configuration and the db are checked, which is about the same).
Also it could be used (added back since it was removed) for some installer page texts. Now it should work as the new 'st' is more reliable, and has some more checks.)
The other options, for the *real* dx improvement would be checking for an existing cotainer inside 't', and fallback to 'st' or similar code when not available.... It won't looks so nice from inside because t() would need to be 'installer aware' too.
(But anyway, now I read what I've written, I think yes, let's go for the real DX improvement....)
Comment #6
Gábor HojtsyDoes st() do anything then? If there was no DIC, the .po files would not be read either since that is all moving to be accessed the via the DIC, right? So sounds like you can wrap things in st() but it would not do anything anyway(?).
Comment #7
Jose Reyero CreditAttribution: Jose Reyero commented> the .po files would not be read either since that is all moving to be accessed the via the DIC, right?
Not atm, though I don't know whether that's planned for the future.
If you look at the other patch, in the last vers ion, st() actually does file translation (and custom string overrides) without needing a container.
Comment #8
Gábor HojtsyWow, ok!
Comment #9
ParisLiakos CreditAttribution: ParisLiakos commentedi think we can get rid of st() as well, all it needs is some adjustement in the installer (moving the before database container creation to the top) and st() can die as well, i am gonna try it later
Comment #10
Gábor HojtsyWoot, that would be the ultimate solution :)
Comment #11
Gábor Hojtsy#1813762: Introduce unified interfaces, use dependency injection for interface translation landed!
Comment #12
Gábor HojtsyAlso moving onto sprint!
Comment #13
ParisLiakos CreditAttribution: ParisLiakos commentedi am gonna try get rid of all of them
Comment #14
ParisLiakos CreditAttribution: ParisLiakos commentedComment #16
ParisLiakos CreditAttribution: ParisLiakos commented#14: drupal-remove_get_t_st-1838310-14.patch queued for re-testing.
Comment #18
ParisLiakos CreditAttribution: ParisLiakos commented#14: drupal-remove_get_t_st-1838310-14.patch queued for re-testing.
Comment #19
ParisLiakos CreditAttribution: ParisLiakos commentedits green:)
what i did..added a minimal temporary container so t() does not fail
i think it is acceptable since it kills st()
Comment #20
tstoecklerSince #1813762: Introduce unified interfaces, use dependency injection for interface translation landed this is actually a (IMO major) bug (at least if we can agree that rolling that back would be a super bad idea):
This issue is the correct solution to this problem: Prepare the container early enough before anyone wants to translate anything at all. The current patch does in fact fix the problem.
Aside: I have no idea why PoStreamReader wants to translate its errors and at first glance it seems like a particularly bad idea. Especially since it's in \Drupal\Component it's not very nice to call get_t() randomly...
Comment #21
ParisLiakos CreditAttribution: ParisLiakos commented+1000
Opened #2017345: Drupal\Component\Gettext\PoStreamReader calls get_t()
Comment #22
tstoecklerThis will need a re-roll due to #2004506: Move standard_country_list() to Drupal\Core\Locale\Country. :-(
Comment #23
ParisLiakos CreditAttribution: ParisLiakos commentedreroll
Comment #25
ParisLiakos CreditAttribution: ParisLiakos commentedforgot to
git rm core/modules/locale/lib/Drupal/locale/Tests/LocaleInstallTest.php
Comment #26
Gábor HojtsyDid not review each single line, but skimming through the file, there are no unrelated changes, use of $t, st and get_t are removed, t() is universally introduced and the functions themselves are removed. What's not to like? :)
Comment #27
catch#25: drupal-remove_get_t_st-1838310-25.patch queued for re-testing.
Comment #29
ParisLiakos CreditAttribution: ParisLiakos commentedreroll
Comment #31
ParisLiakos CreditAttribution: ParisLiakos commented#29: drupal-remove_get_t_st-1838310-29.patch queued for re-testing.
Comment #32
ParisLiakos CreditAttribution: ParisLiakos commentedback to rtbc
Comment #33
catchCommitted/pushed to 8.x, thanks!
This will need a (very short most likely) change notice.
Comment #34
Gábor HojtsyWoot, added https://drupal.org/node/2021435
Comment #35
tim.plunkettNot sure why HEAD hasn't failed, but I've gotten this in a couple test runs.
Comment #36
ParisLiakos CreditAttribution: ParisLiakos commentedhead is not broken..tests passed cause those forgotten st() occurences only happen when something goes wrong in installation
i am sorry for missing those
Comment #37
webchickCommitted and pushed to 8.x. Thanks!
Comment #38
PanchoVery nice, but the pre-bootstrap translation of installer interface doesn't seem to catch the default language, so doesn't actually translate the interface...
While I unfortunately didn't manage solving this in the given time, what I did is some refactoring, which I still wanted to share with you:
Also, corrected comments.
But the main bug remains open.
Comment #39
PanchoComment #40
ParisLiakos CreditAttribution: ParisLiakos commentedThis is not going to work, because the minimal container is locked, so cant be reused like that
Can you open a followup for this instead?
Thanks
Comment #41
PanchoHere's the followup: #2022549: Regression: No interface translation in the first installer steps
Comment #42
Gábor HojtsyRemove sprint.