Problem/Motivation

We have to replace all left usages of _l(), just 46 of them ...

Proposed resolution

Do all of them, except 5 calls in:

  1. PathController::adminOverview() We want to link to a PATH (its an path alias), and run path processing to show the alias (sadly this is not covered by unroutedUrlAssembler). We could though fetch the alias manually. Child issue is #2368323: Replace _l() in PathController::adminOverview().
  2. UrlTest::testLinkXss()This is testing _l itself
  3. FieldPluginBase::renderText() (2 calls) We do have an issue for itself #2280961: (Views)FieldPluginBase::advancedRender() calls SafeMarkup::set() on a string that it doesn't know to be safe
  4. \Drupal\views\Plugin\views\field\Url::render() #2368653: Replace _l in all places (3) besides one.

Remaining tasks

User interface changes

API changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Title: Replace all existing _l calls » Replace nearly all existing _l calls
Assigned: dawehner » Unassigned
Status: Active » Needs review
Issue tags: +WSCCI
FileSize
26.47 KB
dawehner’s picture

Status: Needs review » Needs work

The last submitted patch, 1: 2364161-1.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
26.6 KB
4.47 KB

Let's see how much can be fixed with that.

Status: Needs review » Needs work

The last submitted patch, 4: 2364161-4.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
30.82 KB
7.07 KB

There we go.

Status: Needs review » Needs work

The last submitted patch, 6: 2364161-6.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
32.39 KB
4.48 KB

Reroll.

Wim Leers’s picture

Issue summary: View changes
FileSize
32.22 KB

Patch didn't apply anymore. Straight reroll.

Updated the IS with the ones that aren't replaced by this patch. I'm assuming those are very tricky for very intricate/complex reasons.

In any case, this is a big step forward. I'd like to see confirmation from dawehner that those 5 unconverted calls are indeed the ones he intended not to convert. Once he confirms that, this is RTBC.

dawehner’s picture

Issue summary: View changes

Added comments to all the calls.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for the clarifications; this is now RTBC then :)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 9: 2364161-9.patch, failed testing.

mpdonadio’s picture

Assigned: Unassigned » mpdonadio
Issue tags: +Needs reroll

Patch doesn't apply anymore. Few manual merges to check.

mpdonadio’s picture

Assigned: mpdonadio » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
32.24 KB

Applied 2364161-9.patch to eb56c4d352457f, and did rebase. Two manual merges were needed. Needed to merge the use section in LinkFieldTest, and also had to account for the Unicode change in PathController on line 92 (and made sure not to touch line 89 per the proposed resolution above). I don't know what test covers PathController, so couldn't test this change locally.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Manually compared those conflicting hunks; they look good: same logical changes as before. Thanks!

Hence back to RTBC!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Can we get issues filed for 1 and 4 of the remaining calls to _l() - and can relevant CR be updated to link to this issue. Thanks.

mpdonadio’s picture

Issue summary: View changes

Updated CR, and made issue for PathController::adminOverview(), but I don't know why \Drupal\views\Plugin\views\field\Url::render() isn't being updated.

dawehner’s picture

Issue summary: View changes
Status: Needs work » Reviewed & tested by the community
Related issues: +#2368653: Replace _l in all places (3) besides one., +#2368323: Replace _l() in PathController::adminOverview()

Thank you @mpdonadio
Here is the other one. Back to RTBC

alexpott’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Fixed

This issue addresses a critical bug and is allowed per #2350615: [policy, no patch] What changes can be accepted during the Drupal 8 beta phase?. Committed 46e6a32 and pushed to 8.0.x. Thanks!

  • alexpott committed 46e6a32 on 8.0.x
    Issue #2364161 by dawehner, mpdonadio, Wim Leers: Replace nearly all...

Status: Fixed » Closed (fixed)

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