Stop trying to move issue attachments to a subdirectory of files

It's always been a UI WTF, true.
I've got into the habit of either saving then re-editing, or just guessing (based on insider knowledge/experience) where images I put into issue will turn up.
Either way, it does feel like an annoying hack that was never really finished. The objective it is trying to achieve makes (made) sense, but the UI way it did it happens at the wrong point.

No objection to turning it off, but a better solution (using filefield_paths or similar) should remain on the wishlist if/when rebuilding this function.

Hi Derek,

So in an interesting twist, I'm fine with this change, but not fine with our issues upload directory in general. As you note above, the files dir itself has about 4k files in it. This is absolutely fine with ext3+dir_index. However, our issues directory has over 200k files in it. Ext3+dir_index can handle a lot more than this, but we are over the usual recommendation for keeping directories performing well. Sharding this out a bit would be a really good idea.

This fix likely requires D7. Until then, I'm fine with putting these in the files directory.

-N

Monitoring so that we can move forward on #528682: Allow inline images to be posted to Drupal.org project pages, docs pages, and comments without any special permissions... looks like this has a solution that just needs to be implemented now? Thanks!

As mentioned in IRC, I'm not really a fan of this stop-gap fix idea.

The clear separation of issue attachments into /files/issues makes sense. (even though it could be improved, as already pointed out in the summary)

Belatedly changing it now means that issue attachments

1. will additionally pile up in /files
2. the identical issue attachment file name will be able to exist in both directories (/files/screenshot.png and /files/issues/screenshot.png)
3. and thus, even if we wanted to, we'd no longer be able to "merge" the new files back into /files/issues (though moving files is not on the table and generally considered a bad idea)

I think the issue with previews got a bit over-hyped. The issue is not new and known for years already. It can be resolved later, or not.

Postponing #528682: Allow inline images to be posted to Drupal.org project pages, docs pages, and comments without any special permissions on the preview issue sounds like strive for perfection to me, which isn't necessarily needed to make progress. Especially as it requires, from my perspective, a rather large and debatable infrastructural change, which additionally has an impact on every site that is running project_issue.module.

Lastly, the entire issue is going to be obsolete as soon as we've upgraded drupal.org to D7 - which we're hopefully able to do in the very near future.

Regarding the issue postponement, it *is* necessary in my opinion. One of the main reasons we want non-priv users to be able to post issues is so that we can open up more docs pages (with embedded images) for editing by the masses, so people can be encouraged to include graphics in docs without becoming admins. If we have to write complex instructions like:
http://drupal.org/node/528682#comment-4998252
for them to be able to do it, that is *not* OK for this group of users.

greggles just pointed out to me in IRC that people editing docs pages do not have this problem, it is only people uploading images to issue comments. So I guess maybe my objection is not as important.

The clear separation of issue attachments into /files/issues makes
sense.

Not to me. I tend to scatter my files everywhere, so "sense" is relative.

* will additionally pile up in /files

What is the problem with this? The issue nnewton raised indicates that piling them up in a less full dir could be helpful.

* the identical issue attachment file name will be able to exist in both
directories (/files/screenshot.png and /files/issues/screenshot.png)

I can only think of one problem with this - that searching for an issue based on a patch will be harder. I don't thin that's worth stopping this.

* and thus, even if we wanted to, we'd no longer be able to "merge" the
new files back into /files/issues (though moving files is not on the
table and generally considered a bad idea)

So, this means your last point is moot. Great!

Postponing this issue for sharding sounds like letting perfect be the enemy of good enough ;)

Let's do this, but even if we don't I agree we should go ahead with the other.

if not striving for perfection?

drupalorg_project_form_alter(&$form) {
  $form['upload']['#after_build'][] = 'drupalorg_project_upload_after_build';
}

drupalorg_project_upload_after_build($element, &$form_state) {
  foreach (element_children($element) as $key) {
    $element[$key]['#description'] = strtr($element[$key]['#description'], array('files/' => 'files/issues/'));
  }
  return $element;
}

Can we try deploying this on the test site and see how it works? This seems like a sensible fix (and it should probably not be on this issue, which is nominally about not moving the files).

@sun the issue is that the files are not at those paths until the comment/node is saved, so previews would break, and people wouldn't know why, but if they saved that preview with the broken images, the images would magically start working. See #194449: Incorrect preview permalink when attaching files for the discussion about this.

We could do sun's fix and add a description/help text saying "This is the path where this file is expected to be saved. Will not work in preview mode, and subject to change. You may need to edit the comment to fix a path if you use this in a link" or something like that? Would be no worse than the current situation (and I think a little bit better).

@jhodgdon we basically got sign-off on this change from nnewton above, so we don't need anything hacky here, we just need to make project not save the files into a subdirectory.

Looking at the code in project_issue module it seems that the rewriting is surrounded by an if statement:

if ($issue_dir = variable_get('project_directory_issues', 'issues') ) {

So presumably testing this would be as simple as setting that variable to be an empty string? No clever heroics needed here.

RE #13 - nnewton may have signed off, but several others responded saying it was not a great idea after all. So I am not sure whether consensus has been reached.

This is removing a buggy hack, which is less code to think about when upgrading to D7. There is absolutely no reason not to do this.

With infrastructure sign-off, moving to project_issue for implementation.

We have date-based subdirectories on Drupal.org now. The filename deduping was taking too long for interdiff files. And D7’s file field configuration has been good for updating this configuration.