Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ThomWilhelm’s picture

Status: Active » Needs review
FileSize
964 bytes

Replicated issue locally. Submitting proposed patch to fix this issue.

znerol’s picture

Issue tags: +Needs tests

Reproduced on a fresh install, it looks like we are lacking test coverage here, tagging.

star-szr’s picture

Version: 8.0.0-alpha14 » 8.0.x-dev
Assigned: japo32 » Unassigned
Status: Needs review » Needs work

Unassigning, based on the other issues created by @japo32 I don't think they intended to work on the patches. Please correct me if I'm wrong!

Because this needs tests setting to needs work.

star-szr’s picture

Also setting the correct parent relationship.

star-szr’s picture

perennial.sky’s picture

In previous patch l function is directly, but now l function is used with drupal static class as core changes.
Reroll patch, thanks Cottser for mentoring.

perennial.sky’s picture

Status: Needs work » Needs review
jibran’s picture

Issue tags: +SafeMarkup
znerol’s picture

Status: Needs review » Needs work
FileSize
28.13 KB

Still needs tests. Also it seems that the link to new comments is also affected by the same problem.

Granted that cell data takes render arrays, wouldn't it be appropriate to supply a proper '#type' => 'link' element instead of #markup?

perennial.sky’s picture

Here is the patch which remove title and replies section.

perennial.sky’s picture

Yeah i agree with you that we should use '#type' => 'link' element instead of #markup?, if we implement this then we have to change in many places and that's not a feasible way.

Fabianx’s picture

Using #markup is not the right fix.

This just hides the problem ...

The string needs to come from a function that creates safe markup.

Also \Drupal::l is deprecated, so using #type => link is what should happen here.

subhojit777’s picture

Assigned: Unassigned » subhojit777
subhojit777’s picture

Here is the patch according to suggestions from @Fabianx in #12. Will work with tests soon.

subhojit777’s picture

Status: Needs work » Needs review

Invoking tests.

jaime@gingerrobot.com’s picture

Here is what it looks like with patch applied:
Screenshot of text area on activity

jaime@gingerrobot.com’s picture

So I was just looking at this ticket https://www.drupal.org/node/2309737 which asks to remove direct format_plural calls with function from the translation object: IE:
\Drupal::translation()->formatPlural()

So this patch will need to be updated so that it doesn't conflict with the other patch.

subhojit777’s picture

Status: Needs review » Needs work
subhojit777’s picture

Status: Needs work » Needs review
FileSize
2.31 KB
548 bytes

Thanks @jaimekristene for pointing this out. Here is the corrected patch. Can anyone help me with tests. I guess I have to write simpletests for this since we will carry out tests in a virtual instance.

There are tracker_test_views.info.yml and views.view.test_tracker_user_uid.yml, but I think they are for views.

subhojit777’s picture

FileSize
3.67 KB
1.25 KB

Patch with tests added.

subhojit777’s picture

Assigned: subhojit777 » Unassigned
znerol’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

Applied the patch and verified manually that it is working. Also executed the test from #20 locally without the rest of the patch and the new assertions fail, which shows that they are working as intended.

Fabianx’s picture

RTBC + 1 - looks great

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed 390315b and pushed to 8.0.x. Thanks!

  • alexpott committed 390315b on 8.0.x
    Issue #2322439 by subhojit777, akashjain132, znerol, jaimekristene,...

The last submitted patch, 19: titles_in_a_user_s-2322439-19.patch, failed testing.

The last submitted patch, 19: titles_in_a_user_s-2322439-19.patch, failed testing.

Status: Fixed » Closed (fixed)

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