Port project issue text filter

Tagging for Sprint 4.

Going to do this with the field formatter, as they relate to each other.
#1628160: Port issue nid field formatter to D7

Funny timing -- I was *just* reviewing this patch when your comment came in. I'd rather not roll these two into a giant patch, but rather keep them separate. I'm happy to get this in first if your patch for #1628160 is going to depend on functionality in here. I believe the D6 version just re-uses the filter's theme function, so feel free to write the patch for #1628160 to assume this theme function exists. But let's not merge these issues or patches.

Thanks!
-Derek

Reading the patch -- oh yeah, right, comment numbers... hrmph. I had kinda forgotten about that mess. :( Just opened #1632492: Figure out and port project_issue comment numbering functionality to D7 about that (and added it to Roadmap for porting Project* to D7).

There are handful of code-style bugs in here (which were present in the original code, but we might as well fix them as we're changing the code, anyway.

Otherwise, this looks good to me. I'm going to do some local testing, clean up the minor issues, and hopefully commit/push this. Stay tuned.

Actually, a few other problems in here:

A) Instead of $node->type == 'project_issue' we need to use the helper functions, e.g. project_issue_node_is_issue() so that we can handle multiple node types that are all configured to be issues.

B) Why bother defining a no-op prepare callback? Can't we just leave that empty if we don't need it?

C) Shouldn't the process callback use 'process' in the name to avoid possible confusion/collisions? Hrm, I guess the core filters don't do that, so we should just stick with core's convention here.

D) The .' related code style bugs I mentioned above.

I'll work on cleaning these up, stay tuned.

Okay, fixed everything from #5, and pushed commit a2fc848 with all the changes.

I guess I'll just update #1632492: Figure out and port project_issue comment numbering functionality to D7 to leave a note there about fixing that aspect of this functionality when that's resolved. So, let's call this one fixed for now.

Heh, nope. ;) I was only testing with comments. If you use this filter inside an issue node body itself, you get what appears to be infinite recursion, which is weird, since bdragon added code that was trying to defend against this. I don't really want to dig in deeply to debug and resolve this right now (I really need to be making progress on project_release). So, I'm going to assign this back to him.

Meanwhile, at least the theme function is already in place for use at #1628160: Port issue nid field formatter to D7 ;)

Cheers,
-Derek

Note, the recursion problem is in _project_issue_filter(). If you put return $text at the top of that, it "fixes" the problem (by crippling the filter).

More likely, it's really the node_load() inside project_issue_link_filter_callback() since that's trying to load the body which in turn invokes the filter which tries to node_load() which...

So, the recursion defense inside the theme function isn't helping anything. Moreover, ugh -- that's a lot of business logic in a theme function. ;)

dww--- it is the node_load. I found this out Friday, and am trying to come up with a way around this...

I re-wrote the theme function to allow for a better way of handling the recursion. Note: there are still changes in #1628160: Port issue nid field formatter to D7 that will cause the theme function to run better.

Cool, thanks for taking a stab at cleaning up these problems. A few questions:

A) Why did you remove this check?
- if (is_object($node) && node_access('view', $node) && project_issue_node_is_issue($node)) {
We still want that, no?

B) Can't we put the recursion defense inside project_issue_link_filter_callback() instead of the theme function? As I wrote in #8:

Moreover, ugh -- that's a lot of business logic in a theme function.

Seems like a theme function called theme_project_issue_issue_link() shouldn't be responsible for entity_load(), recursion defense, etc. It should just take an issue entity and render a pretty link to it. Seems like that'll make it easier to reuse at #1628160: Port issue nid field formatter to D7. If the weirdness of recursion from the filter trickles all the way down into the theme function it's more spagetti code, and woe unto the poor themer that just wants to change the markup. ;) Generally, theme functions should be as "dumb" as possible, and this one is ridiculously smart. My basic understanding of #1628160 is that there's not going to be any of the same recursion problems, right? Maybe we should hash this out in IRC...

C) We can't really just leave this part undone: + // TODO: Add in way of getting status.

Thoughts?

Thanks!
-Derek

A) We really only need the last bit, re-added.

B) Fair enough. I moved it to the project_issue_link_filter_callback function.

C) This part is actually done, but done in #1628160: Port issue nid field formatter to D7.

A) we should still be checking node_access() on the issue node you're linking to, otherwise, we're opening up an access bypass security hole here. What if you're using this filter on a site with private projects/issues (e.g. security.d.o) where a given user isn't supposed to be able to see the issues from other projects? They could just start fishing for issue titles by previewing a comment with [#N]. That'd reveal to them the title of an issue (e.g. a core security bug or something) that they wouldn't otherwise be able to see.

B) Cool, that looks a lot better, thanks. I'll attempt to test this a bit and see if it's really working, but this seems like the right thing to be doing.

C) Can we move the parts that get this working into this patch? Seems like getting the status is unrelated to the field formatter and should be fixed here as we're mucking around with the recursion bugs and related changes. In case it's not already clear, my goal is that the patch we commit here fixes the filter, the whole filter, and nothing but the filter. ;)

Also:

D) Why rename the theme variable from 'node' to 'entity'? We already gave up on the illusion that we're going to be able to support non-node entities as issues, so this seems a bit silly. But, I guess it's not the end of the world to leave it like this, so I'm open to a discussion about it.

Thanks!
-Derek

Regarding C), let's not strive for patch perfection if there's code in two patches that are both going to be committed and will ultimately accomplish the same thing.

Nobody will be judging the ingredients of each of our patches. They will, however, be judging the final result and how long it took us to get there.

A) Ah, yeah. I see what you mean.

B) ---

C) Yup, moved in.

D) Because I didnt realize that we gave up on that. corrected.

Cool, thanks! Tested this out and it appears to be working fine. Committed and pushed:

http://drupalcode.org/project/project_issue.git/commit/35d5df1

However, upon further consideration, this is just way too much business logic for a theme function. I think it's a lot safer and better to turn this into a theme template with a template_preprocess() function to do the heavy lifting:

http://drupalcode.org/project/project_issue.git/commit/6640da1

While I was at it, I fixed a few minor bugs from your patch:
- Code style on try/catch should match if/else.
- D from #13 was still broken.
- You were defining $attributes directly using $username outside of the if() where we were checking if $username is even defined.

I'm not 100% sure what I've done here (directly stuffing everything into $variables['link'] instead of using separate variables for title, url, attributes, etc) is the way to go, but this certainly seems like a better approach than the nasty we used to do in D6 (which you were just porting, no fault of yours!). If we want to split this apart, let's just do so in a follow-up issue and leave this one fixed, since the basic filter functionality is now ported (and working!) in D7.

Sorry this took so long to get to, and thanks for your persistence!

Cheers,
-Derek