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.

Files: 
CommentFileSizeAuthor
#38 st-1838310-38.patch2.12 KBPancho
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#35 st-1838310-35.patch5.62 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,755 pass(es).
[ View ]
#29 drupal-remove_get_t_st-1838310-29.patch158.32 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 57,563 pass(es).
[ View ]
#25 drupal-remove_get_t_st-1838310-25.patch158.3 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-remove_get_t_st-1838310-25.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#23 drupal-remove_get_t_st-1838310-23.patch156.88 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] 57,530 pass(es), 0 fail(s), and 1 exception(s).
[ View ]
#14 drupal-remove_get_t_st-1838310-14.patch157.15 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 58,234 pass(es).
[ View ]
#3 1838310-remove_get_t-03.patch90.58 KBJose Reyero
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1838310-remove_get_t-03.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

Gábor Hojtsy’s picture

Status:Active» Postponed
Gábor Hojtsy’s picture

Issue tags:+language-ui

Fix tag.

Jose Reyero’s picture

StatusFileSize
new90.58 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1838310-remove_get_t-03.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

This is mostly the patch, that I'm taking out from the big one in #1813762: Introduce unified interfaces, use dependency injection for interface translation

Gábor Hojtsy’s picture

This is going to be a tremendous DX improvement! I'm a bit surprised st() is not taken out in this patch. How come?

Jose Reyero’s picture

> 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....)

Gábor Hojtsy’s picture

Does 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(?).

Jose Reyero’s picture

> 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.

Gábor Hojtsy’s picture

Wow, ok!

ParisLiakos’s picture

i 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

Gábor Hojtsy’s picture

Woot, that would be the ultimate solution :)

Gábor Hojtsy’s picture

Gábor Hojtsy’s picture

Issue tags:+sprint

Also moving onto sprint!

ParisLiakos’s picture

Assigned:Unassigned» ParisLiakos

i am gonna try get rid of all of them

ParisLiakos’s picture

Status:Needs work» Needs review
StatusFileSize
new157.15 KB
PASSED: [[SimpleTest]]: [MySQL] 58,234 pass(es).
[ View ]

Status:Needs review» Needs work
Issue tags:-DX (Developer Experience), -D8MI, -sprint, -language-ui

The last submitted patch, drupal-remove_get_t_st-1838310-14.patch, failed testing.

ParisLiakos’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, drupal-remove_get_t_st-1838310-14.patch, failed testing.

ParisLiakos’s picture

Status:Needs work» Needs review
Issue tags:+DX (Developer Experience), +D8MI, +sprint, +language-ui
ParisLiakos’s picture

Assigned:ParisLiakos» Unassigned

its green:)

what i did..added a minimal temporary container so t() does not fail
i think it is acceptable since it kills st()

tstoeckler’s picture

Since #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):

  1. If st() is called without the existance of a $container (i.e. at the very start of the installation) it calls TranslationManager->translate() directly
  2. That eventually ends up in PoStreamReader->readLine()
  3. In case there are any errors reading the translation stream PoStreamReader->readLine() wants to translate those errors
  4. To translate the errors PoStreamReader->readLine() consults get_t(), which currently returns 't' directly
  5. Calling t() results in $container->get('string_translation'). $container, however, is NULL (see 1.)

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...

ParisLiakos’s picture

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...

+1000
Opened #2017345: Drupal\Component\Gettext\PoStreamReader calls get_t()

tstoeckler’s picture

Status:Needs review» Needs work
ParisLiakos’s picture

Status:Needs work» Needs review
StatusFileSize
new156.88 KB
FAILED: [[SimpleTest]]: [MySQL] 57,530 pass(es), 0 fail(s), and 1 exception(s).
[ View ]

reroll

Status:Needs review» Needs work

The last submitted patch, drupal-remove_get_t_st-1838310-23.patch, failed testing.

ParisLiakos’s picture

Status:Needs work» Needs review
StatusFileSize
new158.3 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-remove_get_t_st-1838310-25.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

forgot to
git rm core/modules/locale/lib/Drupal/locale/Tests/LocaleInstallTest.php

Gábor Hojtsy’s picture

Status:Needs review» Reviewed & tested by the community

Did 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? :)

catch’s picture

Status:Reviewed & tested by the community» Needs work
Issue tags:+DX (Developer Experience), +D8MI, +sprint, +language-ui

The last submitted patch, drupal-remove_get_t_st-1838310-25.patch, failed testing.

ParisLiakos’s picture

Status:Needs work» Needs review
StatusFileSize
new158.32 KB
PASSED: [[SimpleTest]]: [MySQL] 57,563 pass(es).
[ View ]

reroll

Status:Needs review» Needs work
Issue tags:-DX (Developer Experience), -D8MI, -sprint, -language-ui

The last submitted patch, drupal-remove_get_t_st-1838310-29.patch, failed testing.

ParisLiakos’s picture

Status:Needs work» Needs review
Issue tags:+DX (Developer Experience), +D8MI, +sprint, +language-ui
ParisLiakos’s picture

Status:Needs review» Reviewed & tested by the community

back to rtbc

catch’s picture

Title:Remove st(), get_t() and $t for good» Change notice: Remove st(), get_t() and $t for good
Priority:Normal» Critical
Status:Reviewed & tested by the community» Active

Committed/pushed to 8.x, thanks!

This will need a (very short most likely) change notice.

Gábor Hojtsy’s picture

Title:Change notice: Remove st(), get_t() and $t for good» Remove st(), get_t() and $t for good
Priority:Critical» Normal
Status:Active» Fixed
tim.plunkett’s picture

Title:Remove st(), get_t() and $t for good» [HEAD BROKEN] Remove st(), get_t() and $t for good
Category:task» bug
Priority:Normal» Critical
Status:Fixed» Reviewed & tested by the community
StatusFileSize
new5.62 KB
PASSED: [[SimpleTest]]: [MySQL] 57,755 pass(es).
[ View ]

Not sure why HEAD hasn't failed, but I've gotten this in a couple test runs.

ParisLiakos’s picture

Title:[HEAD BROKEN] Remove st(), get_t() and $t for good» Remove st(), get_t() and $t for good

head is not broken..tests passed cause those forgotten st() occurences only happen when something goes wrong in installation

i am sorry for missing those

webchick’s picture

Category:bug» task
Priority:Critical» Normal
Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks!

Pancho’s picture

Category:task» bug
Priority:Normal» Critical
Status:Fixed» Needs review
StatusFileSize
new2.12 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Very 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:

  • The early minimal container seems to be needed just in order to verify database settings? Then we probably don't need to install_register_translation_service() before. So we don't have to register it twice in minimal bootstraps.
    Also, corrected comments.
  • Also, while the container gets overwritten by the regular bootstrap, we don't have to recreate it again in minimal bootstrap.

But the main bug remains open.

Pancho’s picture

Status:Needs review» Needs work
ParisLiakos’s picture

Category:bug» task
Priority:Critical» Normal
Status:Needs work» Fixed

This 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

Pancho’s picture

Gábor Hojtsy’s picture

Issue tags:-sprint

Remove sprint.

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