Problem/Motivation

Apply #2120235: Regression: routing / tabs / actions / contextual links lack way to attach replacement arguments to UI strings to configuration translation derivatives, so we don't need our own custom local task and action classes.

Proposed resolution

- Drop custom plugin implementations.
- Add title arguments to derivatives class.

Remaining tasks

- Review.

User interface changes

None.

API changes

None.

Files: 
CommentFileSizeAuthor
#6 2252735-6.patch7.2 KBpwolanin
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,627 pass(es). View
#6 increment.txt2.5 KBpwolanin
#3 interdiff.txt2.42 KBGábor Hojtsy
#3 use-core-title-arguments-3.patch6.86 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,620 pass(es), 4 fail(s), and 0 exception(s). View
#1 use-core-title-arguments.patch6.35 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,523 pass(es), 170 fail(s), and 3 exception(s). View

Comments

Gábor Hojtsy’s picture

FileSize
6.35 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,523 pass(es), 170 fail(s), and 3 exception(s). View

Diffstats not bad, hah? :)

4 files changed, 2 insertions(+), 110 deletions(-)

Status: Needs review » Needs work

The last submitted patch, 1: use-core-title-arguments.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
6.86 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,620 pass(es), 4 fail(s), and 0 exception(s). View
2.42 KB

The custom class is not needed anymore either. Neither the custom plugin ID which we only used for the title argument lookup which we now do directly. So even less code :)

Status: Needs review » Needs work

The last submitted patch, 3: use-core-title-arguments-3.patch, failed testing.

pwolanin’s picture

Assigned: Unassigned » pwolanin

I'll test this locally to see if the bug is in the tests or the code change and post a new patch

pwolanin’s picture

Assigned: pwolanin » Unassigned
Status: Needs work » Needs review
FileSize
2.5 KB
7.2 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,627 pass(es). View

Looks like it needed the strtolower() call also.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Lovely!

Gábor Hojtsy’s picture

Agreed looks good :) The final diffstat then is: 4 files changed, 3 insertions(+), 114 deletions(-) Woot!

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work

Uhm doh, wait, we cannot actually do this :( The placeholder itself needs to be translated in the page's language, so its not static to the plugin definition... In fact we have this comment in the removed code that reminds us of that :(

...
 // ... We need to retrieve a runtime title (as opposed to
 // storing the title on the plugin definition for the link) because
 // it contains translated parts that we need in the runtime language.
...

So we may not even be able to get rid of these two classes unless we would store a callable as the replacement and then call it which starts getting too much complication for avoiding these classes. Maybe a won't fix :/

Gábor Hojtsy’s picture

Status: Needs work » Closed (won't fix)
Issue tags: -sprint

So yeah this is a won't fix unfortunately :/