File module 4.7 doesn't check whether parcel module is installed before using ec_product_parcel table (see e.g. http://drupal.org/node/87977).

Also an undefined constant FILE_DIRECTORY_TEMP is used as a fallback in case 'file_directory_temp' variable not defined (maybe this constant existed in an earlier version of Drupal)?

The attached patch fixes these two bugs, the first by protecting the offending code in if (module_exist('parcel')) { ... },
the second by calling file_directory_temp() instead.

The patch also also:
- Makes some E_ALL fixes to prevent php Notices
- Adds some inline and API comments to help readability (not all complete, sorry)
- Adds some TODO Notes on things to check, e.g. why admin shouldn't view user files,

Sorry for bundling up multiple issues in one patch. When I get some spare time I'll unwrap into separate patches...

Best wishes,

Mark.
www.PostgraduateStudentships.co.uk - where ideas and funding meet

CommentFileSizeAuthor
#4 file.module_1.patch2.84 KBplumbley
file.module_0.patch6.8 KBplumbley

Comments

Chill35’s picture

Chill35 commented

I think I reproduced that bug.

I am using ecommerce-4.7.x-2.0.tar.gz with the release of Drupal 4.7.4.

My only product is a file product.

In "User Account" when looking at "My files" I get these warnings

user warning: Table 'drupal.ec_product_parcel' doesn't exist query: SELECT st.created, st.expires, stp.title, pp.mnid, p.nid FROM ec_transaction AS st, users AS u, ec_product AS p, ec_product_parcel AS pp, ec_transaction_product AS stp WHERE u.uid = st.uid AND st.uid = 1 AND p.nid = stp.nid AND st.txnid = stp.txnid AND pp.nid = p.nid AND st.payment_status = 2 ORDER BY st.txnid DESC in F:\htdocs\drupal-4.7.4\includes\database.mysql.inc on line 121.

The file I successfully purchased (from myself, as admin) on my local machine, with rebates that made the download free is there and I am able to download it.

I will try that patch and tell you if the warning disappears.

Chill35’s picture

Chill35 commented
Version: 4.7.x-1.x-dev » 4.7.x-2.0
Status: Needs review » Reviewed & tested by the community

No more warning messages.

About this :

- Adds some TODO Notes on things to check, e.g. why admin shouldn't view user files.

As admin I can see which items were bought in each transaction. I completed a transaction as a user (uid not 1).

Great work!!

neclimdul’s picture

neclimdul
He/Him
commented
Status: Reviewed & tested by the community » Needs work

Thanks for the patch its got some good things. Could you do me a favor and do a couple things though.

A) Implementation of hook_productapi() is sufficient documentation for file_productapi(). This keeps us from having to keep up with documentation changes across all module hooks and is inline with core documentation.
B) like you said, divide it up. I'll be happy to commit just the code fixes and the documentation as 2 patches. We wont be as strict as core on this issue but it helps us go through more patches if we're able to look at a smaller definable group of things with each patch. :)

For the latter, could you make a separate issue so they can each be addressed separately?

plumbley’s picture

plumbley commented
Status: Needs work » Needs review
StatusFileSize
new file.module_1.patch2.84 KB

IMHO I don't believe "Implementation of hook_something" should be recommended practice, since it separates the documentation from the implementation into different files and hence goes against JavaDoc-like principles of keeping them together and hence less likely to get out of sync. (The comments here are not complete though. The standard header in here was for my reference when tracing through ecommerge though, so was not yet tailored. It was included in the patch becase that's as far as I got by the time I spotted other people were having this problem so the patch might be useful).

Example: transition from 4.6 to 4.7 left some modules claiming "Implementation of hook_validate() when they still tried to change the node, when this action should now be in hook_submit(). If they actually had some documentation at the top explaining what they *thought* they were doing, it would might been easier to spot the documentation mismatch, and hence interface implementation mismatch. But also the 4.7 documentation still incorrectly claims that hook_validate() "can also be used to make changes to the node before submission", and there is no documentation for hook_submit(), so its a bit of a mess.

Anyway, as you suggest, that's probably for another issue, so I've edited the patch to remove the comment sections, leaving this code fixes. Hope this is OK...

Best wishes,

Mark.

brmassa’s picture

brmassa commented
Status: Needs review » Closed (fixed)

Guys,

the File module has been rewritten for eC4.

regards,

massa