Problem/Motivation
t()
is one of the last places (other than SafeMarkup::format()
) where we still use the safe list. Currently, use of the safe list in t() leads to bugs as well as security issues (see #2549943: [plan] Remove as much of the SafeMarkup class's methods as possible).
Proposed resolution
Instead of a string, make t()
return a TranslationWrapper
object implementing SafeStringInterface
. This object will return the translated string upon casting to string using the __toString()
magic method.
Doing this will allow us to get rid of the safe list entirely as the patch to get rid of SafeMarkup::format()
will be trivial once the current issue is solved (see #2559971: Make SafeMarkup::format() return a safe string object to remove reliance on a static, unpredictable safe list).
Remaining tasks
Review current patch
Review draft change record at https://www.drupal.org/node/2564451
Commit
User interface changes
No
API changes
This will be disruptive to contrib in that the return value of t() will need to be explicitly cast to string in strpos() and in array keys. The conversion will be easy however as all we need to do is (string) t('String'), and the array key conversions will mostly be about labels in #options.
Comment | File | Size | Author |
---|---|---|---|
#225 | 2557113-3-225.patch | 54.64 KB | alexpott |
#225 | 223-225-interdiff.txt | 765 bytes | alexpott |
#223 | 2557113-3-223.patch | 54.69 KB | alexpott |
#223 | 214-223-interdiff.txt | 8.63 KB | alexpott |
#214 | 2557113-3-214.patch | 50.06 KB | alexpott |
Comments
Comment #2
alexpottLet's see what breaks. Drupal at least installs
Comment #4
alexpottThis is going to be too painful... :(
Comment #6
joelpittet@alexpott I think we worked around this somehow in the original green SafeMarkup(as an object). May take a bit of digging to get at it but could prove to have some gems. #1825952-132: Turn on twig autoescape by default #132 is the passing version as an object before the switch to $safe_strings list
Lol, this looks very, very familiar:) I wondered where this would pop it's head again with the direction we are taking. Was kinda hoping it would have magically been fixed by some other issue... anyways maybe the work we did originally will prove fruitful.
Comment #7
alexpott@joelpittet yeah - this doesn't feel like something we can do before 8.0.0 - I wish it was.
Comment #8
stefan.r CreditAttribution: stefan.r commentedI had a look at this yesterday to see if I could get rid of the most common test failures as they were mostly about the tests themselves. This breaks everything but reduces the 494 failures to a more reasonable number, maybe looking into this further will unearth some bugs or at least unnecessary t()s?
Comment #9
claudiu.cristeaWhy not perform the cast inside
t()
?Comment #11
alexpottre #9 the whole point of this issue is to not do that. TranslationWrapper objects are safe for Twig to print - if we cast to a string we lose the safe-ness.
Comment #12
joelpittetMy thoughts exactly! :)
Here's a failing patch with less failures.
Comment #14
alexpottWow only 122 fails... I just worry about contrib given what we have to do fix stuff here...
I think only the second hunk is needed...
This shouldn't be necessary now you've changed Html::getClass right?
I think in this and other cases we should just remove t()
Comment #15
catchHmm the actual changes in actual modules are pretty slim here - looking at shortcut, locale, entity reference each having to make a change or so each.
Comment #16
catchComment #17
stefan.r CreditAttribution: stefan.r commented@joelpittet nice work! Just wondering how did we go from 54 failures to 93 failures on CI? Looking at the interdiff that number should have gone down instead of up, did the fixing of fatals unveil more failures? :)
Comment #18
stefan.r CreditAttribution: stefan.r commentedThis here was causing a lot of those new failures ;)
Comment #19
stefan.r CreditAttribution: stefan.r commentedLet's see how many fails we have now
Comment #21
stefan.r CreditAttribution: stefan.r commentedComment #23
joelpittet@stefan.r oh yeah I should have left that in, but I was going to next find out the source of that :) I've been making slow headway in an ignore issue. Feel free to use it too if you expect the patch to fail #2465399-64: IGNORE: Patch testing issue
Comment #24
joelpittetThese changes will need to be removed somehow. We don't pass variables(from the docs on t() say) in t().
https://api.drupal.org/api/drupal/core!includes!bootstrap.inc/function/t/8
Comment #25
stefan.r CreditAttribution: stefan.r commentedOuch, had missed that! Let's discuss that one on IRC - translating the label could be tricky.
Comment #27
stefan.r CreditAttribution: stefan.r commentedLet's see if this fixes another couple of fails
Comment #28
stefan.r CreditAttribution: stefan.r commentedSo now that this is green this this doesn't look horribly bad after all! Now we need to get rid of all the hacks/@todos...
Some points that will need addressing:
We had double escaping here.
var_export choked on this during assertEquals() because of the translation wrapper in the label property of the context definition.
Without this we had XSS on output in XssBlockTest, maybe #2558791: "!"-prefixed tokens should Xss::filterAdmin() but not affect safeness would solve this?
TranslationWrappers are not considered a primitive type, so this failed the constraint on the subject field.
Comment #29
joelpittetAwesome work! if the power returns to my part of Vancouver tomorrow I'll see if I can't tackle some of those todos with you.
Comment #30
alexpottI think this might mean we can get rid of the safe list - since converting SafeMarkup::format to return a SafeString will be way simpler.
Comment #31
alexpottCreated #2559969: Make SafeStringInterface extend \JsonSerializable so we don't have to do so much string casting that uses the
JsonSerializable
trick discovered here for all SafeStrings.Comment #32
stefan.r CreditAttribution: stefan.r commentedNits
Comment #33
stefan.r CreditAttribution: stefan.r commentedre #30 - #2559971: Make SafeMarkup::format() return a safe string object to remove reliance on a static, unpredictable safe list has a patch
Comment #34
dawehnerGeneral question, should we document why we have to string casts in some places?
@todo = new issue or @todo as part of this issue?
+1 for this change
mh I don't get why we need t() here for !on_label / !off_label
What about using translateWithoutArgs? Mh I see we want to express that we return an actual string
IMHO for xpath we can always cast to string, right? anything else would not pack anyway, given how xpath works
What about using strval for iteration?
Comment #35
stefan.r CreditAttribution: stefan.r commentedlol... yes that was wrong :)
Comment #36
joelpittetRe #34.3
I'm going to go with -1 on this change. Because it causes the other
!on_label
stuff. Those values though must be used someplace in an inappropriate way for things to fail. I think that is where we need to focus on finding where the source of that inappropriate use(may it be using the values flipped as keys or something like that).Comment #37
chx CreditAttribution: chx commentedWe have shied away from doing this because DX. Having to (string) every translated string to be usable as a string is a nuisance. Not sure whether big enough. Also, wouldn't this make Drupal a hair slower?
Comment #38
catchI think anything we lose we could more than make up by fixing issues like #2497275: ~50 calls to t() for two strings in LanguageManager() on every request.
Also for issues like that, the translated string is never used, so the creation of the TranslationWrapper object should be cheaper than doing the actual translation - so should be a net improvement if we're still doing things like that.
Then on most pages with SmartCache we shouldn't really be calling t() even once for cache hits.
So overall it'll be a net improvement when we unnecessarily translate strings that won't be rendered, or no change for warm caches, and possible a small hit on cold caches but I'd expect very minimal there.
Comment #39
stefan.r CreditAttribution: stefan.r commentedWe could split out some of the smaller issues here:
- The test helper methods that cast the assertEquals() etc comparisons to string
- The boolean label issue
- The casting of array keys to string
- Some of the unnecessary t()'s in tests
- The storing of translated strings in array keys in Tasks.php
On IRC it was also mentioned that
TranslatedString
may be a better name thanTranslationWrapper
here (we could alias and deprecate).But first all core committers may need to be on board with this issue, otherwise not sure how we'll get the smaller issues in?
Comment #40
alexpott@stefan.r I've written to all the other committers about the possibility of removing the safe list and why I think it is a good idea. I think the list of issues you propose breaking this up into make sense. Some of them are worth doing regardless, eg, the boolean label issue, the removal of some of the unnecessary t()'s in tests, and the storing of translated strings in array keys in Tasks.php.
Comment #41
joelpittet+1 to the split up and
TranslatedString
sounds way more specific to me so +1 there but may be an unnessasary BC break?Comment #42
stefan.r CreditAttribution: stefan.r commentedit's not a BC break if we alias and keep the deprecation until 9.0.0...
Comment #43
stefan.r CreditAttribution: stefan.r commentedComment #44
joelpittetAh yeah good call:)
Comment #45
stefan.r CreditAttribution: stefan.r commentedSplitting off some issues:
#2560733: Remove a few problematic t() calls from tests
#2560729: Cast objects implementing SafeStringInterface to string in TestBase / WebTestBase
#2560719: Remove usage of t() string in the array keys for the database install tasks
Comment #46
stefan.r CreditAttribution: stefan.r commentedAlso added:
#2561259: Cast (optgroup) array keys that may hold translated strings to string
#2561273: "Text on demand" for "input required" exposed form plugin is not displayed
#2562155: Cast TranslationWrapper objects in config to string during Config::set()/setData()
Comment #47
stefan.r CreditAttribution: stefan.r commentedI just reviewed this patch again to see if it could be split up into further sub-issues, but it doesn't look like that will be needed. Once the current sub issues land this will be quite reasonable / review-able in terms of size.
Comment #48
stefan.r CreditAttribution: stefan.r commentedUpdating the issue summary, and let's leave the rename for later
Comment #49
stefan.r CreditAttribution: stefan.r commentedComment #50
stefan.r CreditAttribution: stefan.r commentedAdding a link to the draft CR in the issue summary
Comment #51
Gábor HojtsyComment #52
stefan.r CreditAttribution: stefan.r commented@Gabor I htink this is just waiting on the children (#2561273: "Text on demand" for "input required" exposed form plugin is not displayed, #2561259: Cast (optgroup) array keys that may hold translated strings to string) and a decision from #2506427: [meta] !placeholder causes strings to be escaped and makes the sanitization API harder to understand but that shouldn't have to block further work on this - technically we could already do a reroll with those three patches included, and as soon as they are solved we can do a reroll that should result in a much smaller patch.
Comment #53
stefan.r CreditAttribution: stefan.r commentedReroll of #32 that includes some test fixes and the following 3 issues:
#2561273: "Text on demand" for "input required" exposed form plugin is not displayed
#2561259: Cast (optgroup) array keys that may hold translated strings to string
#2566447: Cast safe string objects to string or use assertEqual() in some assertIdentical() comparisons
Also posting a combined patch that includes just those 3 issues as well as an interdiff with this patch, just so we can continue working on this issue and see what would be left as soon as the other 3 went in.
Out for the evening so just posting my progress in case anyone wants to look any test failures or @todos
Comment #58
stefan.r CreditAttribution: stefan.r commentedLet's try this again
Comment #59
stefan.r CreditAttribution: stefan.r commentedOK so this is green now, just to reiterate the actual patch that would be for this issue is in 'differences.txt'. Some 6 remaining @todos:
Discussed this with @plach and we thought it'd be reasonable for objects with __toString() to pass the primitive type validation as the object can be cast to a primitive... So we could do something like this in
PrimitiveTypeConstraintValidator
instead:This just needs fixing - it may have been fixed already in HEAD so we can try taking it out and see if it passes tests.
Needs documentation.
This may not be needed? It is only making 1 test fail: https://www.drupal.org/pift-ci-job/31711
This may not be needed anymore.
Needs investigation, something seems wrong here.
Comment #62
joelpittetEdit: LOL whoops wrong issue...
Comment #63
stefan.r CreditAttribution: stefan.r commentedJust posting an updated patch (combined with the patches from the 2 child issues). As to the previous 6 @todos, just one left (number 6):
1. Fixed the constraint validator
2. Already fixed in HEAD.
3. Not needed anymore.
4. Was not needed. We were translating an already translated string ("The site's default language (@language)"). The nice thing about t() returning an object is we can now check if a string has already been translated - so the patch now does that in Drupal\views\Plugin\views\PluginBase::listLanguages()
5. Not needed anymore.
6. Will investigate.
Comment #64
stefan.r CreditAttribution: stefan.r commentedDiscussed with @joelpittet and that last @todo is an XSS in the tests when
$view->label() == '<script>alert('view');</script>'
:He'll work on this in #2567159: Fix improper usage of t() in ViewsBlock
Comment #65
chx CreditAttribution: chx commented@stefan.r asked me whether a SafeStringInterface object should pass as a string for StringItem .
Well, look at that. What is StringItem? It's an attempt to add metadata and behavior to a PHP data type that doesn't have it because PHP data types are utter shit. What is SafeStringInterface? It's an attempt to add metadata and behavior to a PHP data type that doesn't have it because PHP data types are utter shit.
Um. On what grounds should StringItem not acknowledge its brother in the struggle for better data types? Obviously it should.
Comment #66
stefan.r CreditAttribution: stefan.r commentedSo per #65 this looks to be OK then. Thanks @chx!
Comment #67
stefan.r CreditAttribution: stefan.r commentedThe 3 child issues will be dealt with in:
#2567159: Fix improper usage of t() in ViewsBlock
#2561259: Cast (optgroup) array keys that may hold translated strings to string
#2566447: Cast safe string objects to string or use assertEqual() in some assertIdentical() comparisons
So excluding those, what is left of the patch is the attached txt, which is ready to be reviewed now as there are no further @todos left, other than deciding what to do with the placeholder replacement code in TranslationWrapper::render().
Comment #68
xjmBased on discussion with @alexpott and others, I'm tentatively promoting this issue to critical. It helps address numerous problems with SafeMarkup simultaneously, including:
!placeholder
(see discussions in the related critical #2506427: [meta] !placeholder causes strings to be escaped and makes the sanitization API harder to understand)@placeholder
to also work for non-HTML contexts (see #2509218: Ensure that SafeString objects can be used in non-HTML contexts)Comment #69
lauriiiThere is nothing that could be worked on right now. Feel free to unpostpone this if you want to keep reiterating patch provided on #67 before child issues are fixed.
#2567159: Fix improper usage of t() in ViewsBlock
#2561259: Cast (optgroup) array keys that may hold translated strings to string
#2566447: Cast safe string objects to string or use assertEqual() in some assertIdentical() comparisons
In the latest patch there is one @todo which is blocked by decision made in here: #2506427: [meta] !placeholder causes strings to be escaped and makes the sanitization API harder to understand
Comment #70
stefan.r CreditAttribution: stefan.r commentedThanks. In the mean time #67 could still use further review (or iterations).
Comment #71
dawehnerJust some small comment here and there
Can we document that you should use ::translate() instead?
We could name it a bit more explicit and use translateToString for example
I'm curious whether the part of SafeMarkup::format() and this which is identical could/should be moved to something?
I'm curious whether its really the best idea to remove the t() here and in other places ... just imagine that people take code like this as example
Did we discussed whether we should take care of that internally?
Comment #72
stefan.r CreditAttribution: stefan.r commented@dawehner regarding 5, IMO translating an already translated string is almost always a bug... So we could do an assert/exception/trigger_error and/or cast to string in the TranslationWrapper constructor when it is passed in an already translated string, but this feels a bit out of scope in a critical so the conservative thing to leave that as is, just fixing the calling code in order to fix the test fail, what do you think?
Comment #73
lauriiiRemoved docs added in #2561259: Cast (optgroup) array keys that may hold translated strings to string
Comment #74
dawehnerStefan_r I agree, it should fail somehow early
Comment #75
stefan.r CreditAttribution: stefan.r commentedThis addresses @dawehner's feedback other than point 4 which would still need either a comment or some refactoring so we can use a t() in those unit tests.
Comment #78
alexpott@lauriii / #69 I don't see how #2567159: Fix improper usage of t() in ViewsBlock blocks this. It looks like just a regular followup.
Comment #79
stefan.r CreditAttribution: stefan.r commented@alexpott without that patch we have an xss test fail due to the !placeholders
Comment #80
stefan.r CreditAttribution: stefan.r commentedpatch fail link at https://www.drupal.org/pift-ci-job/32145
Comment #81
stefan.r CreditAttribution: stefan.r commentedComment #82
stefan.r CreditAttribution: stefan.r commentedComment #83
xjmRegarding #37 and #38, I think this is probably important enough to merit profiling.
Comment #84
xjmI've postponed #2509218: Ensure that SafeString objects can be used in non-HTML contexts on this issue.
Comment #85
dawehnerRan some quick profiling for the frontpage, no page cache, no dynamic page cache.
So given that I think its pretty much the same at least for this arbitrary example, given that xhprof has sometimes a bad time profiling function call overheads.
As you see, we gain 30 function calls (so probably ~10 things which are translated (the new() call + render() + getStringTranslation())
Comment #86
chx CreditAttribution: chx commentedSo what are we profiling and why? I'm just asking for future criticals and policy and stuff.
Comment #87
dawehnerIMHO even if it doesn't change anything, having some knowledge about how much each particular patch changes is valueable.
Comment #88
dawehner<3
Those calls IMHO should call out to the TranslationManager directly and translateToString() as this makes it clearer what the test is about.
Comment #89
benjy CreditAttribution: benjy at PreviousNext commentedIt seems weird to me that a trait lives in the utility namespace. A utility for me is a final class with static methods.
PlaceholderTrait looks and feels like a utility, why does it have to be a trait? So we can do static:: vs Placeholder:: ?
Comment #90
dawehnerWell, its near SafeMarkup ...
Fair, well it is used probably by SafeMarkup and TranslationWrapper, but yeah the question is whether you would ever want to extend the behaviour simply by subclassing.
Comment #91
stefan.r CreditAttribution: stefan.r commented@benjy the documentation on that trait still needs some work, it wasn't actually intended to be called directly through a static call other than from tests, SafeMarkup::format() and TranslationWrapper::render()
Comment #92
benjy CreditAttribution: benjy at PreviousNext commentedI'm not sure the difference between a trait and a class with static methods should be if you ever want to extend it. I think traits offer a benefit if they help you fulfil an interface, if all the methods are static it should just be a utility class.
Comment #93
catchThe trade off on function calls will be between t() calls that never get translated (we still have several in HEAD including on warm caches), vs. overhead of translating the ones that do.
This happens both on warm and cold caches to varying extents, so is probably why it doesn't look like much change, although in reality it's likely to be quite a big change in what's actually happening.
Another thing to note here is that if we eventually get to the point on warm caches of not translating anything in a t() call at all (because all the ones that are rendered are render cached), we'll end up saving the locale cache get for non-English requests. So this could end up a genuine net improvement eventually.
Comment #94
plachAn internal method on an interface looks very weird, this makes me wonder whether it would make sense to extract the
::translateToString()
logic to another trait so it can be protected and does not need to be part of the public API.80 chars
Comment #95
stefan.r CreditAttribution: stefan.r commentedreroll now that all children have been committed
Comment #96
stefan.r CreditAttribution: stefan.r commentedComment #99
dawehnerI think for documentation its fine to reference a different package
It is a big question whether the translation wrapper should handle that internally. What about adding an assert() into the TranslationWrapper now? This could allow us to find potential issues much faster, especially in contrib in code
This just feels wrong, can we at least try to get rid of that madness in a follow up? The placeholder handling does not belong into this value object. We could easily move it into the TranslationManager itself
I'm curious how this ever happened. Does that mean there is code doing $entity->title->value = t('Foo')? Seems like a bug to do that in the first place.
Missing docs, sorry
Comment #100
alexpottConsidering we're removing
!placeholder
I would do this differently and have $safe be a parameter passed in by reference. Rather than a multi value return.Not used
I don't think we need to use
static
here -$this->
should be fine.Let's call
->render
to make it more explicit - the random string cast looks very odd.$arguments is no longer used.. can be removed above.
Comment #101
stefan.r CreditAttribution: stefan.r commented@dawehner
1. It is :)
2. Added that assert(), along with a string cast.
3. Moved it into TranslationManager::translateToString() - letting you refactor this further if you like
4. We need this for default subjects, ie t('No subject;)
5. Docs added.
addressed #100 as well
Comment #103
dawehnerThis looks really great. Thanks for fixing this!
Nice!
Do we want both the assert and the cast? I think we should actually check that we don't translate something which was already marked as safe, don't you think so? It should be just pure string allowed to be passed in here.
nice!
Aren't there more cases in that particular file?
Comment #104
stefan.r CreditAttribution: stefan.r commentedtest fixes
Comment #106
stefan.r CreditAttribution: stefan.r commented#103.4 - the others were already converted, that was just one last stray (string) t(). Also removed the cast from the TranslationWrapper constructor which does a stricter is_string() check now.
I haven't fixed that last test failure yet, was it introduced by #2506445: Replace !placeholder with @placeholder in t() and format_string() for non-URLs in tests?
Comment #108
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThis patch is looking great, but I think the above is now a problem. Because it means that all t() results (which with this patch, now return the wrapper) are considered safe. But whether a TranslationWrapper::__toString() is actually safe depends on the arguments, at least until #2506427: [meta] !placeholder causes strings to be escaped and makes the sanitization API harder to understand is resolved. Technically, TranslationWrapper shouldn't even implement SafeStringInterface until then, but that would be a pain to make this patch have to change and then later change back, so maybe an interim solution can just be to add something to SafeMarkup::isSafe() to handle the possibility of a TranslationWrapper being unsafe? Maybe an @internal TranslationWrapper::isSafe() method that we can remove later?
Comment #112
joelpittetThis feels like a step too far. Is this needed? It's a value object. It's a string wrapped in an object. We aren't translating anything to a string we are translating one string to another string and holding those in a wrapper.
can't see a reason for this change.
Seems like a nice addition +1
Performance I assume?
Why is this needed?
hopefully an unnecessary change.
Edit: 1, 2 and 6 are the same thing I noticed.
Comment #113
stefan.r CreditAttribution: stefan.r commented@effulgentsia yes that was a last remaining @todo - it was made explicit in earlier code comments but those have been removed.
But maybe the result of today's call about the !placeholder problem will be a solution that doesn't involve having to deal with safeness to begin with? I.e. if the result of SafeMarkup::format() / t()->render() is always "safe", this all ceases to be a problem.
Comment #114
stefan.r CreditAttribution: stefan.r commented@joelpittet regarding 4, I just couldnt think of any reason for us to want an empty string in a translationwrapper. It think empty(t('')) would be FALSE otherwise. Just noticed it actually still would be - I don't think there is any way to make an object instantiation return a string, so do we need a TranslationWrapper::create() instead?
5 is needed because we want to retain the old behavior (of performing the actual translation)
Comment #115
dawehner... this method is on the TranslationManager, which for sure, should be able to also return a string. Its the responsibility of an API to be easily used in other places, which aren't part
of the direct Drupal ecosystem, in which we know using those things as array keys don't work, but for external APIs you might not be able to change. At the same time for those, you don't care about safety at all, like for REST API wrappers as example.
Comment #116
joelpittetWe expect the translated string to be safe. So if in all cases we expect it safe REST API doesn't need to care and just treat it as a string. No?
The test failures seemed to be a result of the assert() and maybe getLabel in derivatives in cache. Didn't dig down the callstack but I guess we do have some wrappers coming in from some systems.
Comment #117
dawehnerRight, this is the reason we need an API to do that. translateToString() is the one.
Comment #118
stefan.r CreditAttribution: stefan.r commentedComment #119
stefan.r CreditAttribution: stefan.r commentedFurther regarding the comment about the safeness of the output in #108, #2559971: Make SafeMarkup::format() return a safe string object to remove reliance on a static, unpredictable safe list has the same problem. If output from "!" were always "safe", or use of it triggered an error, the problem would be gone.
Comment #120
alexpottWe also need to look at for things like
format_size()
. This code is completely broken in head since the str_replace will lose safeness - so if translators included any HTML for instance changing the direction then HEAD is broken.Comment #122
stefan.r CreditAttribution: stefan.r commentedSeems the test fail in #104 was because of a test that was fixed without fixing this in the config_translation module itself as well:
Is this already being dealt with in some other issue?
I also wonder if the assert() is not a bit disruptive, it breaks drush which has double translated strings.
Comment #123
dawehnerYes, see #2568781: Replace remaining !placeholder for Non-URL HTML outputs only for $entity->label()
Comment #124
stefan.r CreditAttribution: stefan.r commented@dawehner should we mark that one as a child and blocker of this issue?
Comment #126
almaudoh CreditAttribution: almaudoh commentedReally great work going on in this issue!! Just a few comments and nitpicks:
I'm afraid casting this to string may not be very helpful in debugging.
Nit: the function name implies label is a string when actually the label is a TranslationWrapper being cast to string.
I'm wondering where the opening
<em>
is at. Is it not needed for the substring test?Comment #127
dawehnerIt totally is if you have a backtrace available, AFAIK. Once you have that you could put a breakpoint in there and be pretty much done, aren't you?
Comment #129
stefan.r CreditAttribution: stefan.r commentedAdding a BC layer for !placeholders to return a string along with a unit test. Create a postponed issue for whenever "!" is deprecated: #2569007: Remove "!" backward compatibility layer for t()
@almadoh:
1. Actually it is quite helpful as it outputs the string, it seems we have a couple of them in core as well, see the test fails: https://www.drupal.org/pift-ci-job/34109
Which is making me wonder if we should take out the assert() and address those in a follow-up, @dawehner what do you think?
2. Changed the method name.
3. Not needed.
Comment #130
stefan.r CreditAttribution: stefan.r commentedComment #133
stefan.r CreditAttribution: stefan.r commentedI assume the test fails are about the ! BC compatibility layer - we have a problem when we combine ! and %, for instance in the FormValidator:
"!name cannot be longer than %max characters but is currently %length characters long."
Comment #134
dawehnerMh, well, I think it would still add value, people might just not know how
assert()
actually works.Comment #135
stefan.r CreditAttribution: stefan.r commentedMaybe making this conditional on safeness of the value of the !placeholder will help
Comment #136
dawehnerProper unit tests in order to know exactly what how works (basically for documentation purposes) would be really nice!
Comment #137
xjmThe CR at https://www.drupal.org/node/2566447 is a great start. I think we can add a bit more. E.g. the
strpos()
item should also apply to virtually any other PHP function that operates on a string, right?explode()
or what have you. (Thatsearch_excerpt()
thing from #2568611: Replace remaining !placeholder for Non-URL HTML outputs only in search_excerpt() comes to mind.) I also think a few examples for fixing tests would be good; I added #2566447: Cast safe string objects to string or use assertEqual() in some assertIdentical() comparisons to the issue list for the CR to start.Also, we should clarfy when the developer doesn't have to change anything. Would it be true to say "In most cases, PHP will cast the TranslationWrapper to a string automatically when needed."? And then we could give some examples of code that doesn't need to be changed?
Comment #138
alexpottCreating a followup #2569069: Replace TranslationWrapper with TranslatableString and deprecate TranslationWrapper
Comment #139
alexpottAt least on PHP5.6 that strpos example in https://www.drupal.org/node/2564451 is bogus.... and PHP 5.5.18. Are we sure about this?
Comment #140
alexpottYep - it is only when used as the second argument - which is quite often hardcoded... https://3v4l.org/sjtsH
Comment #141
alexpottI've fixed the CR wrt to
strpos
and added a bit more detail.Comment #142
alexpott@xjm no that the strpos thing does not apply to explode... https://3v4l.org/E2C1G
Comment #143
alexpottThe thing with strpos is that $needle does not represent quite what people think it does - from https://secure.php.net/manual/en/function.strpos.php
Comment #144
plachI was skimming through the change record and I wondered whether it would make sense to make TW implement ArrayAccess to mimic string array access.
Comment #145
chx CreditAttribution: chx commentedMay I offer http://www.phpwtf.org/objects-tostring-are-not-strings
Comment #146
alexpott@plach I don't think so... we've got this far without it. It would also force translation to occur at that point. I think it is better to be more explicit about this with a string cast.
Comment #147
chx CreditAttribution: chx commentedAdded https://www.drupal.org/node/2564451/revisions/view/8885025/8885099 -- notably there was a subtitle there " Array access to a string" it's not, it just happens to use the same syntax but it's really not an array access, it's string access. I have seen this recently elsewhere. Is there a tutorial or heaven forbid a PHP manual page spreading this madness? If the latter, I can fix it, just point it out please. Regarding emulating this syntax with ArrayAccess, what for? Getting a character from a t()'d string makes no sense since there's no way to guess what it would be as it gets translated. I added notes both to string offset access and strpos regarding the futility of these exercises.
Comment #150
plachVery good points, thanks :)
Comment #154
dawehnerI think its okay to not document possible placeholder types in here, given that its just an internal api?
So given that the patch continues to be green, does that mean we will have no double escaping caused by converting from ! to @?
Nitpick: wrap 80 chars could be done different.
Does that comment btw. still makes sense atm.?
Should we document why we need this bit?
Do we really need those "conversions" as part of this patch?
Nitpick: Let's use assertInstanceOf
Comment #155
larowlanthe circularity in making translateToString public concerns me $string_translation->translate() returns $wrapper $wrapper->render loops back to $string_translate, can't quite articulate my exact concerns, will mull over it, wonder if we can somehow use a closure to resolve it..and avoid making translateToString public…
Comment #156
cilefen CreditAttribution: cilefen commented#154-3: Removed the comment. It makes no sense in the context of the function.
#154-6: done
The others are still in play.
Comment #158
stefan.r CreditAttribution: stefan.r commented@dawehner
1. I think so too.
2. The patch shouldn't introduce any new double escaping in strings with !. Not following what this has to do with @?
3. Removed in last patch.
4. This was to prevent having a TranslationWrapper of an empty string as that should never be needed - but it is not the right solution as (empty(new TranslationWrapper('')) == false), still. Can object instantiations return a string somehow? Do we need a helper for this?
5. Not sure, they were causing test fails earlier but maybe the BC layer fixed this now.
6. Fixed in last patch.
Comment #160
alexpottThe patch attached inverts the dependency of TranslationWrapper to the translation service by adding a renderTranslationWrapper method to the translation service.
Comment #162
dawehnerI certainly like the new idea, it helps to get rid of the crazy dependency chain. Its still calling kinda out from one to the other ...
I still thing you should be able to do it without going through the object itself, but well, I'm okay with not fighting this, as I don't have the power / time to do so.
Nice!
Comment #164
alexpottFixing most test fails - this includes #1789090: Warnings when exporting templates: addcslashes() expects parameter 1 to be string which this patch has somehow uncovered... interesting.
Comment #167
dawehnerIn case someone tries to debug this failure. The problem seems to be that on saving translations and
_locale_refresh_translation()
is called.In there though for some reason
\Drupal::service('locale.storage')->getStrings(array('lid' => $lids, 'type' => 'javascript'))
is empty.Comment #168
catchWith HEAD I can't see any strings with 'javascript' context at all.
Enabled locale, enabled Arabic, switched to arabic.
Browsed around including the views UI.
Reminds me of #2421323: Parse js files from library definitions during rebuild to minimize variable_set() calls although clearly the test coverage breaks.
However in the test:
Nothing in {locales_source}.source has .js in it, so this returns FALSE, which means that when the strings get filtered it shows all strings, which means that the first string translated is not JavaScript, which means no javascript file should be created - so the test in HEAD ought to fail afaik.
I have no idea how the test passes in HEAD either, but I think it's the test that's broken, and looks likely that js translation is also broken.
Didn't debug all the way to the end though so take this with a pinch of salt.
Comment #169
alexpottOk wow...
Drupal\locale\Tests\LocaleTranslationUiTest::testJavaScriptTranslation()
in HEAD...Return false... this query is complete nonsense the location is stored in a completely different table. But the first string it can translate is a javascript string because format_size is completely broken. This patch fixes that... so the first string in not a JS string. Fixing the query fixes the tests. This is also why #1789090: Warnings when exporting templates: addcslashes() expects parameter 1 to be string is required because we are detecting a plural translation we were before. Nuts.
Here's how the test looks when it is doing
HEAD
Patch
Comment #172
alexpottSomething is up with DrupalCI
Comment #173
MixologicSorry bout that. Bungled the rebasing of my pull request for a drupalci dependency.. basically I forgot to commit the lock file. Should be well now.
Comment #174
dawehnerIndeed.
FQCN
Did we considered the alternative which is adding an UnsafeTranslationWrapper without implementing SafeStringInterface but __toString?
In case you like to, we could revert those hunks, they should not be needed for this issue, aren't they?
Just in case you like, we could add one test case which has multiple placeholders, @bar and !baz. If we truely don't think of the implementation this might be a good idea
Why is pretending everything not being safe the right choice? At least we should explain that
Comment #175
alexpottSo one thing I think we've missed is formatPlural and friends. These return the output of SafeMarkup::format and hence will be objects once #2559971: Make SafeMarkup::format() return a safe string object to remove reliance on a static, unpredictable safe list lands but should we introduce a
PluralString
object to separate it out? Obviously if we decide to do that it should be a new issue. Also removing the test added by incorporating #1789090: Warnings when exporting templates: addcslashes() expects parameter 1 to be string since as the test fails show this patch introduces implicit test coverage. If this issue lands first (and it probably will) then we should repurpose that issue to add explicit test coverage.Comment #177
alexpottpift at pifr!
GET http://ec2-52-88-182-40.us-west-2.compute.amazonaws.com/checkout/user/login returned 0 (0 bytes).
Comment #179
dawehnerThank you alex!
Comment #181
stefan.r CreditAttribution: stefan.r commentedVery nice work, this looks great!
I think these were being addressed in another issue (#2568781: Replace remaining !placeholder for Non-URL HTML outputs only for $entity->label())
Comment #182
dawehner#2568781: Replace remaining !placeholder for Non-URL HTML outputs only for $entity->label() its adressed different there, because, well, this fix is also IMHO not the right thing to do. We should not need it just to concat those two bit.
Comment #183
plachLooks great, I have only minor complaints:
::renderTranslatedString()
This should be
\Drupal\Core\StringTranslation\TranslationManager::renderTranslatedString()
. Also, unnecessary trailing dot.Since the goal is to remove
!
, can we remove the$safe
parameter and temporarily move the related check inSafeMarkup::format()
?Oh, above I suggested to implement exactly this also for
SafeMarkup
:)Can we use
static::placeholderFormat()
here?It would be nice to add a note to the class doc block about the new use cases this patch addresses.
Shouldn't this be
string[]
?Comment #184
alexpottCreated #2570107: Make format_plural() return a PluralTranslatableString object to remove reliance on a static, unpredictable safe list
Comment #185
alexpott#2568781: Replace remaining !placeholder for Non-URL HTML outputs only for $entity->label() landed - I resolved the conflicts and used all the change from there.
Thanks for the review @plach
!placeholder
SafeMarkup::format()
will be fixed after!placeholder
is removed in #2559971: Make SafeMarkup::format() return a safe string object to remove reliance on a static, unpredictable safe listComment #186
plachWe are still missing the class name (
TranslationManager
).Well, it was for consistency: we normally call static methods with the
static::
form, also in this patch we do it.Comment #187
alexpott@plach but that
static::placeholderFormat()
is in a static class it has to be. And there is no perf benefit either - https://3v4l.org/ecbvSFixed the other thing.
Comment #188
alexpottIgnore #187 it is the same as #185 oops.
Comment #190
lauriiiNit: shouldn't (optional) be in the beginning?
Comment #191
stefan.r CreditAttribution: stefan.r commentedAdding two assertions and removing the bit @joelpittet and @plach had questions about earlier (don't know that we need to be testing assertions, but anyway:
https://3v4l.org/WvWvG)
If they cause a whole bunch of test fails it may be fine to add them in the follow-up mentioned in #129 instead, otherwise it could be good to add them here.
Comment #192
dawehnerJust a couple of nitpicks.
we do really use yoda style, I think its actually better.
Could be written in one line: if ($args = $tran...->getArguments())
For future lazyness reference, you can use SafeStringInterface::class instead
There is assertInternalType('string', $actual);
Comment #196
plachI've posted a POC patch at #2509218-40: Ensure that SafeString objects can be used in non-HTML contexts, which is relying on this. However it changes things quite a bit wrt the translation wrapper, it would mainly remove/deprecate
TranslationManager::renderTranslatedString()
in favor of aTranslationManager::getTranslatedPattern()
method. Since @catch thinks it's worth to explore that solution, would you please have a look to it and think whether there's any change we can do here to alleviate potential BC breaks if we end-up going that way?Comment #197
stefan.r CreditAttribution: stefan.r commentedReverting #191 and created a new issue for those asserts and the one in #129: #2570285: Make sure TranslatableMarkup accepts string values only
Right now
empty(t('')) === FALSE
which is a break with previous behavior, but discussed with alexpott on IRC and this is a limitation we can live with.Comment #198
stefan.r CreditAttribution: stefan.r commentedper #192 reverting to previous status
Comment #199
alexpottI think we can mostly remove placeholdering from TranslationManager and that'll make work in #2509218: Ensure that SafeString objects can be used in non-HTML contexts easier.
Comment #200
alexpottMoved placeholdering to TranslationWrapper and also self injected the TranslationManager into TranslationWrapper which makes TranslationManager into the object factory it should be and way less \Drupal::container magic. Also we now store the result of translation manager on the object so printing the same TranslationWrapper object twice is cheap especially when we context switch.
Comment #201
plachInterdiff looks awesome to me :)
If we happen to reroll this...
...can we skip the FQCN here?
Comment #203
dawehnerTemporarily this adds more logic into TranslationWrapper but once we have #2509218: Ensure that SafeString objects can be used in non-HTML contexts this is moved into its own logical instance, yeah!
Comment #204
dawehnerPHP :(
vs
Comment #205
stefan.r CreditAttribution: stefan.r commented@dawehner that's crazy... let's update the CR as well? Are other array manipulation functions affected?
Comment #206
dawehnerI don't think so. Its behaviour is documented:
Comment #208
dawehnerLet's fix the rest of the failures.
Comment #211
dawehnerComment #213
alexpottStoring TranslationWrapper objects in session does not seem like a good idea. Also there's no need for StringTranslationTrait in TranslationWrapper anymore.
Comment #214
alexpottA here's a nice new KernelTestBase (tng) to test this behaviour.
Comment #223
alexpottThis really is a find all the issues in core type patch... now I discovered we are passing render arrays to drupal_set_message :)
Comment #225
alexpottMessed up the namespacing of the new test.
Comment #226
plachI think this is ready now.
Comment #227
xjmDo these get scanned for translations the same way that
t()
itself does? I assumed it would, but then found no mention of the class on https://www.drupal.org/developing/api/8/localization.I left @Gábor Hojtsy a message asking to clarify.
Comment #228
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI believe so, since
new TranslationWrapper
is a pattern that's existed in HEAD for a while now. Gábor himself authored https://www.drupal.org/node/2540620 when a recent thing got converted to use it.Comment #230
effulgentsia CreditAttribution: effulgentsia at Acquia commentedLooks like potx support for TranslationWrapper was added a year ago: #2280843: Add support for D8 TranslationWrapper, so we should be all good there.
Comment #232
effulgentsia CreditAttribution: effulgentsia at Acquia commentedBot happy again, so back to RTBC.
Comment #233
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI read through this patch again top to bottom and think it's absolutely terrific! Here are the only things I found to complain about, but none of them need to block commit. In fact, I'll probably commit this in the next hour or two if no one beats me to it or asks me not to.
I didn't investigate this to see if it makes sense. I'm guessing it does, but if someone thinks otherwise, please say so or open a follow-up if it's after this lands.
I don't think it's clear from the method name or docs that this only translates the first parameter of t(), and does not do any placeholder replacement. A follow-up to improve these docs would be good.
Can be shortened to
$options +=
.I think we can remove this comment now, since after this patch, we're pretty much using TranslationWrapper everywhere, not just in low-level subsystems.
I'm not clear on what "development aid" means here. This is now pretty much required, since
$attributes[$foo] = t($bar)
is quite common.Must we cast to a string (and thereby do the translation right now)? Could we instead keep it an object, and change the if statement to
if (is_object($value) && !$value instanceof SafeStringInterface) {
?This could be another good thing to add to the change record. The CR already mentions the $needle param in strpos(), but perhaps that can be expanded to also say that any invocation of a PHP function that treats scalar strings and stringifiable objects differently is affected.
Here and the other two places: why an explicit ->render() instead of casting to a string?
Here and the other tests with this same test method: we should probably also change the returnValue to 'Example @test' (or maybe even 'Example translated @test').
Comment #234
effulgentsia CreditAttribution: effulgentsia at Acquia commentedAlso, in some earlier comment, @dawehner thought this wasn't ideal because it ends up putting logic (the details of each sanitization strategy implementation for each placeholder type) into what is otherwise a pure value object. If anyone wants to open a follow-up to move that into a service or whatever, please go for it.
Comment #235
effulgentsia CreditAttribution: effulgentsia at Acquia commentedAdding credit to a few more people who helped drive some aspect of this issue forward. Apologies if I left someone out.
Comment #237
effulgentsia CreditAttribution: effulgentsia at Acquia commentedPushed to 8.0.x and published the change record. Thanks everyone!
Comment #238
alexpottThis is just fantastic... What started out as a crazy idea has come a long way quickly. See you in the SafeMarkup::format() issue to remove the static safe list.
Comment #239
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedThis patch broke site install via Drush. I created a quick fix for Drush in https://github.com/drush-ops/drush/pull/1619
Comment #240
tstoecklerThe Drush problem points to a larger issue. Because of http://php.net/manual/en/language.oop5.object-comparison.php#71623 basically in lots of places where we used to be comparing to strings we might now be comparing objects and are potentially subject to these very weird fatals. Not good...
Comment #241
tstoecklerThe Drush problem points to a larger issue. Because of http://php.net/manual/en/language.oop5.object-comparison.php#71623 basically in lots of places where we used to be comparing to strings we might now be comparing objects and are potentially subject to these very weird fatals. Not good...
Comment #242
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedCreate #2571375: Remove TranslationManager dependency from LanguageManager as a follow-up.
Comment #243
BerdirAnother follow-up: #2571593: Fatal error: Cannot use object of type Drupal\Core\GeneratedLink as array in Tableselect.php on line 220, I think this was caused by this issue too. This broke a bunch of contrib tests, most were easy to fix with some string casting( t() in array keys, t() used with strpos) but that one has to be fixed in core.
Comment #244
Gábor HojtsyIt is unfortunate that this fixed #1789090: Warnings when exporting templates: addcslashes() expects parameter 1 to be string too but the commit credit from there (reyero, vijaycs85 and droplet) did not end up in this commit. (Not that this does not happen from time to time, just taking a note here).
Comment #245
stefan.r CreditAttribution: stefan.r commentedAllowing this to take objects as a first parameter seems to have introduced bugs such as:
Seems we had an object pointing to a nonexistent class there from and older drupal version - casting to string would at least make this fail earlier.
This issue was always supposed to either cast $this->string to string or throw some kind of error if it's not (see #101) - sadly we did neither and we won't have time to get the error check into 8.0.0. Can we at least get the string cast into the RC? Wrapping other objects as input is not well tested and IMO asking for problems.
The relevant issue is #2579121: Cast TranslatableMarkup input values to string, it is inconsistent with FormattableMarkup
Comment #246
Gábor HojtsyYay for security and consistency! Thanks all for your amazing job on this one!
Comment #248
Wim LeersThis caused a fairly nasty regression: #2606460: Regression caused by #2557113: Enabling CKEditor for a text format that did not have it previously is broken.