Closed (fixed)
Project:
D7 Media
Version:
7.x-1.x-dev
Component:
Code
Priority:
Major
Category:
Feature request
Assigned:
Unassigned
Reporter:
Created:
11 Jan 2011 at 18:29 UTC
Updated:
28 Jan 2011 at 03:00 UTC
Jump to comment: Most recent file
Comments
Comment #1
aaron commentedComment #2
aaron commentedWithout this function in a release, I suspect we'll begin to see some otherwise unnecessary code duplication in all the provider files (youtube, vimeo, flickr, brightcove, etc), as we begin to pull in remote streams that haven't yet been saved to the db. Thus, I'm bumping it up to major.
However, as it's not used in the core of Media, it's a little tricky to review. I suggest using the devel module, with something like the following at /devel/php (assuming the Media: YouTube module is installed). Test it both before and after adding the media to the database.
Comment #3
aaron commentedWhile we're at it, perhaps we need to follow the core model for entities (at least nodes) and add ->is_new or some other flag if the object doesn't currently exist in the db...
Comment #4
aaron commentedthough that likely belongs in file_uri_to_object().
for that matter, file_uri_to_object() should probably take care of all this anyway. it's been over a year, and i brought that up at http://drupal.org/node/685818. too bad we didn't push that more in core...
Comment #5
aaron commentedthinking about it more, i think we need to push all of this into file_uri_to_object(). then that will be the callback of choice, and as it already exists in the module, it won't cause as many issues going forward. new patch forthcoming.
Comment #6
aaron commentedthis patch moves it to file_uri_to_object() and adds ->is_new when needed. i'll update the core issue as well, though it's obviously too late to include that there for d7.
Comment #7
aaron commentedThe cool thing is that this change should also make it easier to get rid of that annoying "You have entered a URL for a video that is already in your library." message.
Comment #8
aaron commentedthis should be testable as well. not necessary for a release obviously. updating the tags.
Comment #9
effulgentsia commentedI like it, but I'm nervous about breaking BC of code that calls file_uri_to_object() expecting a new $file (with no 'fid'), especially since file_save() invokes different logic based on whether an fid is set. Would this approach work for you, or does it fail to achieve what you're intending to make possible?
Comment #10
aaron commentedyes, that would be acceptable. thanks, @effulgentsia. should be safe in my use-case, as the file objects shouldn't be saved at that point in the game.
Comment #11
effulgentsia commentedI committed this to HEAD (changed #9 to use isset() where needed), but I agree with #8 that tests are still needed.
Comment #12
effulgentsia commentedComment #13
effulgentsia commentedIgnore the spam. Just trying to organize the issue queue a bit.
Comment #14
effulgentsia commentedComment #15
effulgentsia commentedResetting issue attributes to match original issue. Since we're so low on test coverage, I'm tracking tests that are needed as their own tasks. The one for this is #1026538: Write unit tests for file_uri_to_object(). Eventually, with any luck, we'll have good baseline test coverage, and can then require patches to come with their own tests, as is the standard policy for Drupal core.