Previewing issue follow-ups requires "administer nodes" permission

yes, i have the same problem since a few days

please fix it!
Thanks

I'll add my "me too" here. It's really annoying. Does anyone have the slightest idea what exactly is broken?

I agree

I have no idea what actually is broken. Please attach screenshot.

There's really nothing to take a screenshot of. Here are some steps to reproduce:

1. click "follow up" link on this issue.
2. type something in the "description" field.
3. click "preview".
4. observe that you can't see a preview of the text you typed.

You can also observe the same results when attempting to preview a new issue. (create content - issue)

I've attached a screenshot of what happens when I try to preview this comment.

I think I have found the bug, _phptemplate_default() (in phptemplate.engine) has:

 if ($file) {
    extract($variables);           // Extract the vars to local namespace
    ob_start();                    // Start output buffering
    include($file);                // Include the file
    $contents = ob_get_contents(); // Get the contents of the buffer
    ob_end_clean();                // End buffering and discard
    return $contents;              // Return the contents
  }

Something to do with project_issue seems to set a $file in the $variables array, which then extracted to the local namespace by extract(), overwrites the local $file variable which is used for the template name.

When issue preview is done, two of the same warnings are produced:
Warning: _phptemplate_default(0): failed to open stream: No such file or directory in ...\themes\engines\phptemplate\phptemplate.engine on line 336

In my case it's trying to open a file called 0 (of type integer)

This is a known bug and has a patch in the queue:
http://drupal.org/node/28576

It should proabably be changed in both the project module (don't use the $file variable), and in PHPTemplate (see the patch attached to the other issue).

Committed patch #28576 to HEAD, and upgraded the phptemplate.engine on drupal.org.

I didn't look at #28576 as I was focusing more on project module. I wasted 30mins identifing a known bug that wasn't even directly related to the project module. Oh well, at least I'm learning more of Drupal's internals.

I think this is fixed now. I tested on drupal.org, works for me... Reopen if there are still problems left.

This seems to be broken again. Steps to reproduce are the same as above.

It's only broken when replying to an existing issue. When creating a new issue, preview works just fine.

I can confirm this.

moving

this problem has been driving me nuts, so i've been trying to debug it. unfortunately, i can't reproduce this bug at all on two different test sites running the latest cvs. one is my macos 10.3.9 laptop, running Apache 1.3.33, PHP 4.4.1, MySQL 4.1.18. the other is my shared hosting account, Debian 3.1, PHP 4.4.2, MySQL 5.0.18. in both cases, i could preview followups to existing project issues.

therefore, i don't think it's a bug in project, i think it's something bizzare going on @ drupal.org. i've asked on IRC a couple of times, and no one claimed they could reproduce this (though no one confirmed they tried, either). :( however, given that previews on issue follow-ups work fine for my test sites, i don't see how this is a bug in project. sorry, killes, i'm moving it back. if you (or anyone else) can reproduce on a test site, please include details here and move it again, but at this point, i don't see how else to debug it from a project.module standpoint. :( i think someone who knows about the inner workings of drupal.org needs to figure out what's different about that environment that could be causing this (theme/CSS badness?, bad interaction with another module that's calling form_alter() or something crazy?, i'm just grasping at straws here...)

thanks,
-derek

Ok... hate to bounce this back, but I've reproduced this issue ;-)

It's a permissions issue related to the Project module.

On my system I can reproduce (and cure) this symptom simply by granting and revoking the "administer nodes" permission.

Don't have time now to track it any further, but reproducing on a cvs system is easy using a uid != 1 user.

perfect! thanks. i'll work on a patch later today, if no one else takes care of it before then. [kicks-self-for-not-testing-as-non-root-users]. sorry killes!

-derek

this is a symptom of a much deeper bug. :( after much debugging, i've found that the problem comes deep inside the FAPI mojo. the basic call stack i traced is below. i'll label call sites with [filename:lineno] to hopefully add some clarity to what i'm talking about...

A) project_comment_page() [start of the call chain i was tracing]
B) project_comment_form() [modules/project/comment.inc:18]
- (sets an '#after_build' element to call node_form_add_preview())
C) drupal_get_form() [modules/project/comment.inc:71]
D) form_builder() [includes/form.inc:114]
E) node_form_add_preview() [includes/form.inc:425] [where we're invoking $function()]
F) node_preview() [modules/node.module:1666]
G) node_access() [modules/node.module:1736]
H) node_get_base() [modules/node.module:2271]
I) _node_names() [modules/node.module:241]

this is where the ultimate trouble lies. the "node" in question we're trying to preview isn't a valid node type at all, it's the special-case "project_comment" type. project_node_info() does not return this as a valid node type, and the form we're passing around through all these layers doesn't have a 'type' element at all. so, _node_names() fails to find a valid name, returns an empty string, and node_access() fails, and that failure goes up the call stack such that you end up with an empty 'node_preview' element in the form inside node_form_add_preview(). the reason it works for some users (anyone with "administer nodes") is that in there's a special case for "administer nodes" permission inside node_access() [it returns TRUE immediately, before attempting to check any of the per-node stuff that's going to fail]. and in node_preview(), once node_access() return TRUE, it's happy to preview a "node" even if it's not a node at all...

looking at comment.module, i see that it had to provide its own special case comment_form_add_preview() to get out of this mess. this is the callback it registers inside its #after_build element. this way, comment.module doesn't fall into the trap of using node_form_add_preview(), which is doomed to fail.

it seems like there are 3 possible solutions to this:

1) implement our own project_comment_form_add_preview()
2) convert project to use real comments, not this weird special case quasi-type that causes much grief
3) turn project_comments into real nodes

#3 seems like a very bad idea, for all the performance reasons that led to project_comments (and comments, for that matter) not being full-blown nodes.

#2 seems like a huge amount of work, and there are a bunch of unresolved problems (see #18920 for more on this).

#1 appears to be the only valid short-term solution, but that's a pain in the ass, too, and duplicates a bunch of code.

UGH!

for now, i'm just going to let this issue fester here and gather feedback/comments from other folks. how should we proceed?

thanks,
-derek

someone in IRC asked why this used to work. previously, drupal.org's project was running 4.6, which is pre forms API. all this add_preview() stuff is new in 4.7, which is why it's now broken. i suspect no one noticed before because any of us trying to use project on a 4.7 test site were trying it from a user with "adminster nodes" permissions, and we were hitting the special case in node_access() that allowed it to work. :( either that, or no one bothered testing preview very well. ;)

i finally nailed down hunmonk and we discussed this. it seems my option #1 from comment #21 above is the only viable solution at this point. the main downside of this approach is that it will be (mostly) duplicated code with comment_form_add_preview(). the right thing to do would be to refactor comment.module's code so that the comment_form_add_preview() functionality can be abstracted and made available to both comments and project issue follow-ups. it's *way* too late in the 4.7 game for such a change in core. however, since project is critical for drupal.org (and other sites, including ones i'm working on), i'm just going to make a patch that duplicates the relevent code in project, and in 4.8 we can worry about refactoring comment_form_add_preview() (if that even still makes sense -- we might finally convert project to use real comments by then, for example).

so, i hope to work on a patch for this in the near future, though i just got back from 2 weeks of vacation, and have a ton of catching up to do (and i have to be at a conference for work all next week). but, i'll try to get this done asap. i'd happily review and test someone else's patch if it magically appeared in the meantime, but i'm guessing if i don't write this, it'll be quite a while before anyone does...

-derek

ok, after a failed attempt to take the easy way out and try to get node_preview() working, i dug in and implemented this in what appears to be a sane, working manner:

• added project_comment_form_add_preview()
• split out the code in project_comment_view() that displays an individual comment into _project_comment_view_one(), so it can be shared by project_comment_view(), which calls it in a loop to print all the follow-ups to a given issue, and project_comment_form_add_preview()
• we pass the nid of the issue around in a few places, so we can get to it when needed. e.g. when we're previewing the single followup comment, we show the summary table of what's changing in this post by comparing what's in the form vs. what's in the node

while i was at it, i also fixed the bug in how we filter comments, reported in #58926, since i was touching all the relevent code for that.

TODO: i couldn't quite get the node_load() stuff working properly to stick the rest of the node and comments at the bottom of the form when previewing, which would be a nice touch, but isn't essential. i left the partially-complete code commented out in the bottom of project_comment_form_add_preview(), in case anyone has any insight as to what i should be doing differently.

let me know what should be changed, or if folks think this is good to commit. thanks!

-derek

at zen/gatsby's suggestions, new patch with the following improvements over the previous:

• better coding style for else (no "} else {" on the same line)
• removed directly using markup (>br<) in the code
• added doxygen comments for the new functions
• moved TODO's into the doxygen comments
• renamed "_project_comment_view_one()" to be "_project_comment_view_single()"
• now uses drupal_unpack() instead of unserialize(). while i was at it, i also consolidated some related code so we only have the code to iterate through the comment fields once.

killes pointed out that #after_build is now supposed to be an array. that helped me realize my test site had a stale copy of form.inc. against the latest in cvs, the old patch prints a warning. new version attached.

whoops, thanks to the patch i committed for #59830 the old patch no longer applies cleanly.

fixes for watchdog() and friends caused the patch to not apply cleanly... keeping up with head.

i fixed the after_build needs to be an array bug in #60119, here's a new patch that applies cleanly.

Thanks Derek for your sustained and insightful attention to this surprisingly complex issuem, and for your detailed explanation.

Your approach is probably the correct and necessary one. It does add a fair bit of new code. I have one question that you've likely addressed.

Could we make the node being validated be the original issue (a valid node type)? Could/should we construct the comment, e.g. via form_alter, as a repost of the original issue, with some added fields?

@nedjo: that seems like a lot of extra code, too. it would also make it so that you don't see only what you're changing about the issue when you preview the followup (since you'd just see the whole issue with your changes in-line, as opposed to how it works with my patch, where you only see what you're changing). furthermore, i think this form_alter() approach would make it so that we *couldn't* include the original issue and all the other follow-ups at the bottom of the page, after the form (which isn't quite working in my patch, but there's code that nearly does it which is commented out for now). if it'd work at all, it'd probably be harder than the few additional lines of changes to get what i have there working. finally, that seems like yet more FAPI complication and wizardry in what's already a very complicated FAPI usage. it seems like hunmonk and yourself (and now, to some extent, myself) are the only people who actually understand how project is using FAPI, and i think the less black-magic hackery we do, the better.

my $0.02,
-derek

nothing to see here, just abusing the 'my issues' feature so I can try and test this tomorrow

Yes, it looks like this patch is the way to go. We need to fix this bug as it's a significant usibility issue. I haven't found the time as yet to give the code a detailed review. On basic looking over it looks correct in that it follows closely what's done in comment.module.

a few improvements over the last version:

• we no longer bother passing around the nid of the original issue's node, since we've always got access to that in the URL.
• i fixed the code (and removed the TODO comment) so that the entire issue and follow-up history always appears at the bottom of the form when composing/previewing further folllow-ups. i just needed to pass extra arguments to node_view() so that it thought it was going to be displayed on a full page, and then the followups get "unwound" and displayed appropriately.

so, i think this is now definitely RTBC. can someone else confirm that? PLEASE! ;)

thanks,
-derek

oh yeah, the new patch. ;)

The way this code deals with formats worries me a little.

+      elseif (isset($data['format'])) {
+        $format = $data['format'];
+      }

+    if ($edit->format) {
+      $data['format'] = $edit->format;
+    }

Not that handling of formats is very sensitive. If the user can by-pass his permission and specify the PHP format to be used, he or she will be able to execute code.

I've yet to investigate your proposed changed in more details, but I already wanted to highlight the unusual way of dealing with the input format.

this aspect of the patch is meant to solve http://drupal.org/node/58926. currently, project doesn't do *anything* with the format at all. all comments are filtered with the default format, regardless of what the user selected (if they have powers to select something non-default). if there's a better, more secure, fail-proof way to store and use the filter, please let me know and i'll be happy to modify the code.

Dries, if you want, i could roll a new patch that doesn't touch the input filter code at all, and leaves the current (somewhat broken, but potentially safer) behavior where the requested filter is completely ignored and all issues and follow-ups are displayed with the default filter. we could move this whole filter discussion back to http://drupal.org/node/58926, where it was originally reported. let me know if you think that'd be better. i just want to see this code committed and deployed on drupal.org. this has been broken for *far* too long IMHO. ;)

I think this is dead code:

      elseif (isset($data['format'])) {
        $format = $data['format'];
      }

Looks like $data is never set.

good catch, dries. i think that was something i was trying in an earlier iteration that became obsolete by the change to drupal_unpack(). honestly i don't remember. either way, yes, it's dead, and now gone. is lucky #7 going to be the one? ;)

upon further discussions and testing, i'm ripping the filter-related code out of this patch that was trying to fix http://drupal.org/node/58926. it's just not working correctly, and it's holding up this otherwise RTBC fix to a critial bug. #58926 is *not* critical, so out it goes. ;)

in IRC, dries gave this the final RTBC. ;) WOO HOO!!! comment.inc revision 1.67. AT LAST. ;)