Upload.module uses the following syntax for system variables that indicate whether or not attachments are enabled for a certain node type: 'upload_' . $node->type. 'upload_' is also the prefix for the module's own settings, like the maximum resolution; this variable is 'upload_max_resolution'. This might conflict with a node type that's called 'max_resolution'. It is unlikely that this will happen, but nevertheless a minor bug that's easily fixed by changing the 'upload_' . $node->type syntax to 'upload_type_' . $node->type.

CommentFileSizeAuthor
#22 upload_270302.patch8.84 KBdrewish
#17 upload_variable_conflict_00.patch9.58 KBxano
#13 270302_13_upload_type_namespace_conflict_D7.patch9.28 KBxano
#11 270302_11_upload_type_namespace_conflict_D7.patch9.22 KBxano
#8 270302_8_upload_type_namespace_conflict_D7.patch7.8 KBmr.baileys
#5 270302_5_upload_type_namespace_conflict_D7.patch7.77 KBmr.baileys
#3 270302_3_upload_type_namespace_conflict_D7.patch7.02 KBmr.baileys

Comments

ztyx’s picture

ztyx commented
Status: Needs review » Active

There is no patch here. Did you forget to attach one?

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented
Version: 6.x-dev » 7.x-dev

I think I did. Moving to 7.x.

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
commented
Assigned: Unassigned » mr.baileys
Priority: Minor » Normal
Status: Active » Needs review
StatusFileSize
new 270302_3_upload_type_namespace_conflict_D7.patch7.02 KB

Setting priority to normal as the risk of name collisions is real. Patch for D7 attached.

I also included an additional test in upload.test to verify that no 'File Attachments'-fieldset is included on node/x/add when attachments are disabled for type x.

Updated the documentation in system.api.php too, since it used $form['workflow']['upload_' . $form['type']['#value']] as example in hook_form_alter.

Refactored the user creation in upload.test: there were at least 4 different locations where admin/web users were created. By creating these users during setup, we don't have to keep creating them (saving time, improving test performance).

damien tournoud’s picture

damien tournoud commented
Status: Needs review » Needs work

We need a small update function to migrate old variables to their new names.

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
commented
StatusFileSize
new 270302_5_upload_type_namespace_conflict_D7.patch7.77 KB

I knew I missed something, thanks for pointing this out...

Attached is a patch that takes care of renaming the existing variables on update.

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
commented
Status: Needs work » Needs review

...and back to CNR.

Status: Needs review » Needs work

The last submitted patch failed testing.

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
commented
StatusFileSize
new 270302_8_upload_type_namespace_conflict_D7.patch7.8 KB

admin test user was missing "Administer content types"-permission.

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented
Status: Needs work » Needs review
mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
commented

argh. Thanks :)

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented
Assigned: mr.baileys » xano
StatusFileSize
new 270302_11_upload_type_namespace_conflict_D7.patch9.22 KB

New patch. This migration path is now sturdier. CONCAT() is not available for all SQL servers Drupal supports. Also, I changed the variable name to 'upload_node_type_' . $node_type, which makes more sense and is more future proof (what if we decide to make attachments available to other content than nodes?).

Next time, please don't add extra tests and stuff in patches that have nothing to do with them. It only annoys other people because you make those patches harder to read.

Status: Needs review » Needs work

The last submitted patch failed testing.

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented
Status: Needs work » Needs review
StatusFileSize
new 270302_13_upload_type_namespace_conflict_D7.patch9.28 KB
mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
commented

This migration path is now sturdier. CONCAT() is not available for all SQL servers Drupal supports.

I thought Drupal handled this through database wrapper functions. I had verified this against MySQL (concat supported natively) and sqlite (supported through a registered function, sqlFunctionConcat). Not sure about Postgres though, so you are probably right.

One thing that the changed update path no longer does is clean up the old variables. Shouldn't these be removed after copying the value to the new variables?

Next time, please don't add extra tests and stuff in patches that have nothing to do with them. It only annoys other people because you make those patches harder to read.

At the time of writing this patch, I felt the extra tests and stuff were relevant and related to this change. In hindsight (and having been exposed to the Drupal way of doing things a bit longer) I can see how I got carried away and went too far with the changes to the tests. I'm truly sorry if this annoyed you or anyone else, that was not my intention. Please feel free to remove the test changes from your patch, and I'll submit another issue to get the commited.

As for the documentation change to system.api.php, I feel this one is relevant, since if we're using a real-world example in document, and when the real-world example changes, so should the docs...

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented
Status: Needs review » Needs work

I thought Drupal handled this through database wrapper functions. I had verified this against MySQL (concat supported natively) and sqlite (supported through a registered function, sqlFunctionConcat). Not sure about Postgres though, so you are probably right.

Not for this kind of stuff, unfortunately. I verified that.

One thing that the changed update path no longer does is clean up the old variables. Shouldn't these be removed after copying the value to the new variables?

You're right. I did put a variable_del() there, but somehow it got removed again in the process.

As for the documentation change to system.api.php, I feel this one is relevant, since if we're using a real-world example in document, and when the real-world example changes, so should the docs...

Good thinking!

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented
Issue tags: +Needs documentation
xano’s picture

xano
he/they
Primary language English
Location Southampton
commented
Status: Needs work » Needs review
StatusFileSize
new upload_variable_conflict_00.patch9.58 KB

The attached patch adds a variable_del() to delete the old variables. This requires one extra query for each variable, resulting in 2+2N queries with N being the amount of variables. The alternative is to use the query below, which would result in 2+N queries, but in more and less maintainable code.

      db_update('variable')
        ->fields(array(
          'name' => 'upload_node_type_' . $node_type,
          'value' => $variables['upload_' . $node_type],
        ))
        ->condition('name', $variables['upload_' . $node_type)
        ->execute();
damien tournoud’s picture

damien tournoud commented

I thought Drupal handled this through database wrapper functions. I had verified this against MySQL (concat supported natively) and sqlite (supported through a registered function, sqlFunctionConcat). Not sure about Postgres though, so you are probably right.

Not for this kind of stuff, unfortunately. I verified that.

It does! If it doesn't, please open an issue.

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented

These are expressions and the DBTNG docs expressively state that those should be used with caution, because not all of them are cross-database platform. Therefore an issue about this is not necessary. I checked MySQL, PostgreSQL and SQLite and CONCAT() and || are being used, but there is no common way of doing this.

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented
Issue tags: +Quick fix

Status: Needs review » Needs work

The last submitted patch failed testing.

drewish’s picture

drewish commented
Status: Needs work » Needs review
StatusFileSize
new upload_270302.patch8.84 KB

re-rolled. the changes look good to me.

dries’s picture

dries commented

Waw, I really think this is a non-issue -- the risk of getting a collision is pretty much zero. It has grown in a big patch. I personally prefer to keep the code slightly more readable than fixing this collision.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

The patch is big because it adds tests and updates the example, the actual changes aren't so big.

AFAIK, it is adviced to prefix all variables with the name of the module. upload.module imho doesn't really follow that convention, because it has only the prefix. This is afaik the only place in core where we have something like this, so I think it should be changed.

About the patch:

Is there a reason you access the database directly in the update function, instead of using node_get_types('names')? Why isn't it possible to do something like:

foreach (node_get_types('names') as $type => $name) {
  // ...
}
drewish’s picture

drewish commented

I'm really not buying the size argument either. The weighty bits are the update function and unit tests.

Status: Needs review » Needs work

The last submitted patch failed testing.

emmajane’s picture

emmajane commented
Issue tags: -Needs documentation

Nothing here has been committed, so nothing to document yet. Please update the tags when the patch has been committed. Thanks! :)

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Needs work » Closed (won't fix)

Upload module has been removed from Drupal 7. Dries's response sounds like "won't fix," so won't fixing.

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented
Assigned: xano » Unassigned