From a project view page, you select "tasks" from the top list of tabs (takes you to page node/xx/tasks).
It doesn't show the icons as it does on page storm/tasks/?project_nid=xx

I assume this is just a bug.

CommentFileSizeAuthor
#11 storm--924002-11.patch2.03 KBjuliangb
#9 924002_page_source.txt52.66 KBtchurch
#9 924002.JPG64.49 KBtchurch
#4 924002.patch1.32 KBtchurch
#1 Screen shot 2010-10-22 at 18.49.58.png267.42 KBMickC

Comments

MickC’s picture

MickC commented
Version: 6.x-1.x-dev » 6.x-1.34
Priority: Normal » Critical
StatusFileSize
new Screen shot 2010-10-22 at 18.49.58.png267.42 KB

Hi,

This page is critical to communicating project status - I've got team of people I need to get started using Storm on Monday.

This page used to work - the whole benefit of using a p system is the 'at a glance' status - but without this page we have to resort to the task view and using filters - ineffective.

This is not a problem limited to the dev version - I'm on the production release. See screenshot attached.

Please help.

MC

tchurch’s picture

tchurch commented
Assigned: Unassigned » tchurch

As far as I know, all fixes/new features are delivered to the dev version. I'm not sure but I think the same patch should work on 1.34 and dev.

I'll see what I can do over the weekend to deliver a patch here which you can use to update your copy of 1.34 temporarily until 1.35 is delivered.

juliangb’s picture

juliangb commented
Version: 6.x-1.34 » 6.x-1.x-dev
Assigned: tchurch » Unassigned

So the icons are not appearing at all? Rather than simply being displayed as text?

6.x-1.x-dev as this is where any fixes will be made.

tchurch’s picture

tchurch commented
Assigned: Unassigned » tchurch
Status: Active » Needs review
StatusFileSize
new 924002.patch1.32 KB

I've had a look and found the problem but I'm not sure why.

It's to do with the check_markup function but I'm not familiar with it. If I remove it, the icons appear.
I'm not too happy about removing this function because it must be there for a reason (I suppose).

Attached it a patch to show what I changed to get it.

You could use this on 1.34 (manually patch it on your own website) as I think the file concerned hasn't been changed since then.

MickC’s picture

MickC commented

Fixed! Thanks for that, but I suppose I should be on stand by if a reason is found for the check_markup function requiring a more permanent solution.

cheers,

Mick

juliangb’s picture

juliangb commented

Sorry to deasign earlier, it was a crosspost.

The check markup is there to prevent xss attacks. Therefore, I am not happy with removing it unless we can back it up and ensure we are still safe.

I will return to this over the weekend.

tchurch’s picture

tchurch commented

I put the following line into the file stormtask.theme.inc:

drupal_set_message('

$form:' . var_export($form, TRUE) .'

');

just inside the function theme_stormtask_tasks.

This showed me the contents of the array being used.

It showed me that the variable used here is a picture not text.
Maybe it's just my browser (google chrome).

It might be relevant.

juliangb’s picture

juliangb commented

Any chance of posting the output from that plus if there is anything returned in the page source?

tchurch’s picture

tchurch commented
StatusFileSize
new 924002.JPG64.49 KB
new 924002_page_source.txt52.66 KB

Not sure if these are useful.
I've attached a screen print of part of the output, showing it as an image and also the actual page source for that whole page (including the whole output).

juliangb’s picture

juliangb commented
Priority: Critical » Major
Status: Needs review » Needs work

OK, I've had a look at this a bit more thoroughly now.

I suggest we do delete the check_markup from this file, but alongside this that we run check_plain inside storm_icon so that any output from that is sanitised and we are protected from xss attacks.

juliangb’s picture

juliangb commented
Status: Needs work » Needs review
StatusFileSize
new storm--924002-11.patch2.03 KB

Patch for the bot.

juliangb’s picture

juliangb commented

The bot likes this, which is good. I think this is the right solution but haven't been able to test. Any tests / reviews?

Basically, the aim of the patch is by securing storm_icon() a little better, we can safely remove the problematic check_markup around the whole thing.

tchurch’s picture

tchurch commented

This looks and tests OK with me.

juliangb’s picture

juliangb commented
Status: Needs review » Reviewed & tested by the community

Great, thanks.

ñull’s picture

ñull commented

I can confirm that this patch works fine here.

juliangb’s picture

juliangb commented

#11: storm--924002-11.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, storm--924002-11.patch, failed testing.

juliangb’s picture

juliangb commented

Fails due to #996512: D6 contrib project tests: [MySQL] Drupal installation failed.. Needs retest once that is sorted.

juliangb’s picture

juliangb commented
Status: Needs work » Needs review

#11: storm--924002-11.patch queued for re-testing.

juliangb’s picture

juliangb commented
Status: Needs review » Fixed

Committed to CVS.

Status: Fixed » Closed (fixed)

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