as it says. however, the actual filepath member still contains the path to the file relative to the drupal root. sorry, first patch on core from cvs, and it screwed up and decided to rewrite the test file.

why bother doing this? re. http://drupal.org/node/361854: Currently we store file paths from Drupal's root and the default files directory is sites/default/files meaning that if you upload files and try to move the files directory you'll have to manually update the filepaths in the {files} table.

test file had to be modified slightly (createfile method directly inserted data into db, so filepath needed to be modified)

i'm getting some funky errors about unexpected hooks being triggered. any ideas?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

George2’s picture

Status: Active » Needs work

correcting status, and file_save is triggering some unwanted hooks apparently

David Strauss’s picture

FileSize
7.13 KB

I like the idea, but the patch still has notable problems. Here's a sane reroll with fixed line endings for further review.

starbow’s picture

subscribe

George2’s picture

just as an update, this questions other parts of file.inc, so the larger problem is being considered

Anonymous’s picture

Status: Needs work » Needs review
Issue tags: +DX (Developer Experience)

Let's see what the test bot says. David's version of the patch might need a reroll with current HEAD.

Status: Needs review » Needs work

The last submitted patch failed testing.

drewish’s picture

Status: Needs work » Needs review
FileSize
7.56 KB

here's a re-roll of david's patch with some comment cleanups.

Status: Needs review » Needs work

The last submitted patch failed testing.

tic2000’s picture

Why is this patch only working on info from the database?
The way it's now, the filepath is stored, but when the administrator changes the files directory path

+      if (strpos($conditions['filepath'], file_directory_path()) === 0) {
+        $conditions['filepath'] = substr($value, strlen(file_directory_path()) + 1);
+      }

will try to remove the new value from the filepath, but that values is not stored in the database, so it won't work. So if you change files directory value you still have to manually change the values in the database.

Instead it should work on saving side, so it stores in the databse the filepath relative to the files directory and not relative to drupal root directory.

+1 on having this one in core.

tic2000’s picture

FileSize
1.35 KB

This is a starting point.
I didn't look too much into it. But this patch allows you to upload files and the filepath stored in the database is relative to files directory.
I'm sure there are a lot of other changes to do, especially on those modules that use directly $file->filepath and not file_create_url or file_create_path in "outputing" the file.

tic2000’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch failed testing.

tic2000’s picture

Tests fails because they expect the full path (including files directory path).
Now, how this should be solved?

1. Do we change file_load_multiple function so it returns the full path, but I don't know in this case if there is a risk of that being added in the {files} table on some update hook.

or

2. We don't change the file_load_multiple and change the test to use file_create_path to get the path as should all modules do if this will be the taken approach.

tic2000’s picture

FileSize
2.87 KB
tic2000’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch failed testing.

aaron’s picture

subscribe

jmstacey’s picture

Keep an eye on #227232: Support PHP stream wrappers as this issue is now related.

tic2000’s picture

Status: Needs work » Postponed
Bevan’s picture

Status: Postponed » Needs work

I don't understand why this should be postponed because of #227232. This change is smaller and simpler than that, and and #227232 will include these changes if this gets committed first or this patch can be rewritten if that gets committed first (and that's assuming that both even get committed).

Admittedly I haven't read all of #227232 (It's very long!), so I may well be missing something.

jmstacey’s picture

Bevan: This issue will be handled transparently by the stream wrappers patch. Additionally the changes will allow us to place an index on the path (uri) column. Ultimately, I think the reason this issue is postponed is because it will create a conflict with the wrappers patch, but I'll let others chime in with their thoughts.

I just glanced at the patch and a call such as file_create_path($file->filepath) would in essence be redundant once stream wrappers are introduced [unless you actually want to assert that the path resides in a valid filesystem directory]. For example, if $file->filepath = "public://myfile.txt", PHP will already transparently handle the URL expansion to "sites/default/files/myfile.txt" via the wrapper implementation. So, we would have just made an unnecessary function call that essentially walks through all possibilities until it fails of finds something that fits compared to just interpolating the path using the appropriate wrapper.

Having said that, I'm impartial either way and it actually might be wise to have this patch ready to go in the scenario that the stream wrappers initiative fails. The code sprint in Philadelphia the weekend after next will be a good indicator of whether or not wrappers will make it into D7. However, as far as I'm aware just a bit of cleanup and more extensive unit testing are needed. The core stream wrappers+registry patch is ready to be reviewed by the community to provide the base of these changes. The original patch has been split into three parts, #517814: File API Stream Wrapper Conversion now contains the changes relevant to this issue.

I just glanced at the last submitted patch and it seems awfully small for what's proposed. Are all locations where $file->filepath used updated to prepend the files directory or call an appropriate function that will do so?

catch’s picture

Regardless of whether stream wrappers fixes the symptom of this or not, this patch should go in - having stale/absolute file paths in the database isn't good even if there's a way 'round it.

Bevan’s picture

jmstacey, It sounds like I need to have a closer look at Stream Wrappers part 2. Does it do away with the files table or modify it significantly? What does it do with file_directory_path() and the file_directory_path variable? Do you think it is going in?

The patch failed testing, assumedly because it didn't cover all uses of the files table or file API. However it aims to allow modules to call the file API with paths relative to the Drupal root, then strips out file_directory_path in front of it. I'm not sure yet whether this is an okay solution. Possibly file API should require modules provide the path relative to file_directory_path.

Bevan’s picture

Title: Removing file_directory_path() in filepath in file table » File table includes file_directory_path()
Assigned: George2 » Unassigned
Category: feature » bug

George2 hasn't been here since February, and this is more a bug than a feature.

catch’s picture

Possibly file API should require modules provide the path relative to file_directory_path.

If we do this, then it has to be a proper API change, we can't be stripping things away sometimes and not others.

jmstacey’s picture

Does it do away with the files table or modify it significantly?

Yes and no. We've created a new table {file} with the schema changes primarily being the rename of column filepath to uri with an index. Every file in the system will have a unique URI. These changes were outlined in stream wrappers comment #111. This was also originally proposed in #329301: Rename {files} to {file} and add unique key to the filepath column. We have not changed the status column as originally proposed and that will probably stay in there, but mark that as a TBD at the sprint when the upgrade path gets a thorough working.

Our initial plans were to leave the {files} table intact to allow contrib modules to upgrade at their own pace. We will be providing an upgrade path for core modules. This plan has not changed to the best of my knowledge, but may as we get into the thick it.

What does it do with file_directory_path() and the file_directory_path variable?

The file_directory_path variable is replaced with a trinity: stream_public_path, stream_private_path, and stream_temp_path if I remember correctly. file_directory_path() has been extended to accept a parameter indicating the desired stream scheme*. It currently defaults to the public wrapper so that we don't have to change the majority of function calls and thereby reduce the patch size.

This area is still in flux. Previously, the single file_directory_path() call determined where the file went [public or private]. Now that this attribute is part of the URI, we will need to go back and extend core capabilities for the user to select how the files will be made available. The granularity of this has not been determined for yet. At the moment a system-wide default value is partially implemented. This might still make use of the file_directory_path variable, I don't remember off the top of my head. We could also allow more fine-grained controls such as allowing each module to decide a default (e.g. profile pictures are private but uploads are public), or we could even technically take a per-file approach and let the user decide on upload. At the present the trend seems to be towards the system-wide default because this keeps things simple and won't cause a radical change from D6 to D7 core. Definitely keep an eye on this area and chime in with thoughts.

* A stream is referenced as scheme://target.

Do you think it is going in?

I think the best determinant of this will be the results of the code sprint in Philadelphia. We'll have several people (and some core folks) hammering on things. Personally, I do think that stream wrappers will make it in, but I can't definitively say in what form that will take. It may just be generic support, or it may be everything and the kitchen sink (a.k.a. part 2). There is little [public] indication from the core committers because things have been changing too rapidly for a serious review. This is why we split the patch up into multiple segments, so now the first part (#227232) can be reviewed at a stable state.

Edit: #519392: File API Stream Wrapper Support (Part 1) is also relatively stable and ready for review, but can't be tested until the first patch is committed.

drewish’s picture

catch / Bevan, as jmstacey points out there are a lot of subtle implications to just removing the files path, especially since we have temporary files and permanent files in the same table. Right now they both can have unique file paths. Private file transfer permissions are based solely on the filepath so simply removing the files directory path opens us up to security problems where a temp file and permanent file share the same relative path. The stream wrapper path will give each a unique prefix to solve this problem.

It's partly my fault that the stream wrapper patch has been languishing, jmstacey has been doing a bunch of work on it and I've been distracted by teh ImageCache in core stuff haven't been able to review it. Dries has expressed interest in getting it in so I think the odds are very good of it happening.

Bevan’s picture

So if I understand correctly, to fix this issue (before streamwrappers goes in) we would need to add a flag to the table for 'temp' or 'public/private', as well as an upgrade path that fixes existing D6 file paths, the respective file API functions and users of those functions in Drupal core. Is this correct?

How significantly would this impact streamwrappers patches and jmstacey's work?

jmstacey’s picture

Bevan, yes, that's roughly what would be required.

How significantly would this impact streamwrappers patches and jmstacey's work?

No impact to #227232: Support PHP stream wrappers or #519392: File API Stream Wrapper Support (Part 1) at the present moment. However, there would be a severe conflict with #517814: File API Stream Wrapper Conversion because both issues cover same problem space.

This issue and the database one I mentioned previously really go hand in hand. The majority of File API part 2 already addresses both of these problems. The code is already written with the exception of comprehensive unit tests and part of the upgrade path.

File API part 2 is really poised to take out 3 birds with one stone. First, we eradicate this issue because the database will not care where a scheme like public:// may actually point. That's handled behind the scenes by the wrapper. Second, we cross out that previously mentioned DB change and get a table that provides better performance because each URI is unique and the column can be keyed/indexed. Finally, the wrappers operate just like writing a filesystem using FuSE, so we can do all sorts of "cool" things like run public and private file systems simultaneously. It's extremely flexible, and anyone can write a wrapper for almost anything without touching a bit of core code. It opens the door to a whole new level of interoperability between the mass of modules that handle media of some kind because they won't need to know the implementation details of a particular resource.

I really dislike using the term return on investment (ROI) with software, but our ROI will be tremendously greater with File API part 2. I honestly cannot envision a scenario where we could run both changes side by side without losing something. The closest situation I can think of is to maintain backwards compatibility so that people don't have to use streams* in the database, but at that point we loose the benefits of #329301: Rename {files} to {file} and add unique key to the filepath column and we completely shutter possible interoperability gains putting us back at square one.

* Technically speaking, even a normal path like sites/default/files/foobar.txt is a stream and invokes PHP's file:// wrapper.

Now, I may be just a little teeny bit biased, but my opinion is that we'll get the most benefit out of a quick wrap-up and polish of File API part 2. Are there any counter points that I'm being a bit slow to catching on to? There's that big one: "what if part 2 doesn't make it in?" That is a big risk. However, the code freeze isn't until September, so I propose one of two things:

1. Let's combine forces and crank out part 2. The code sprint in Philly will give us a good indication as to what's going to happen. If things go South (heaven forbid!), then we have a full month to write up this patch which won't need nearly as much time as the things we're trying to pull off in part 2.

2. Move forward with creating a committable patch for this issue, but hold off any serious consideration for committal until we get a clearer picture of stream wrappers at the end of the month.

What do you think?

Bevan’s picture

Thanks so much for your time and patience jmstacey. That makes sense.

drewish’s picture

Status: Needs work » Closed (duplicate)

marking this as a duplicate of #517814: File API Stream Wrapper Conversion

Bevan’s picture

Version: 7.x-dev » 6.x-dev
Status: Closed (duplicate) » Needs work

This bug might still be worth fixing in Drupal 6.

drewish’s picture

Version: 6.x-dev » 7.x-dev
Status: Needs work » Closed (duplicate)

I don't think that's really practical. It would effectively be a big API change. Reverting the status.

Bevan’s picture

Version: 7.x-dev » 6.x-dev
Status: Closed (duplicate) » Needs work

Drewish; I disagree. The patches in this issue predate #517814: File API Stream Wrapper Conversion, which won't be ported to Drupal 6. Those patches are easily ported to Drupal 6, and also fix a bug in Drupal 6's file API, so may be considered for for commital. Perhaps I should move this into a new issue node/bug report?

drewish’s picture

Bevan, You're welcome to roll a patch to get some committer feedback, but unless you can find a very clever way to do it, I'm going to advocate strongly against this sort of an API change. It would require every contrib module that uses {files}.filepath to be updated.

tic2000’s picture

@drewish
If I remember corectly file_directory_path() checks to see if the provided path it's valid, if it's not it appends the files directory path to it. That will not require changes to other modules, or maybe the change to force them use file_create_url() or other file function to get the path and for sure it will allow for an easy movement of the files directory.

yonailo’s picture

subscribing

Status: Needs work » Closed (outdated)

Automatically closed because Drupal 6 is no longer supported. If the issue verifiably applies to later versions, please reopen with details and update the version.