Upload Field: File Paths

[nilshoerrmann]

Symphony's upload field stores the file path in sym_field_upload but also adds it to the actual entry in sym_entries_*. Why?

If I moved folders on the server an change the path in the section editor, all existing entries will get invalid, because they still store the original path. Why doesn't the field just store the file name in the entries table and combines this on load with the defined path.

[michael-e]

Yep, this is annoying.

[nilshoerrmann]

In the current setup:

• If I change the path in the field settings and don't move the files, all old entries will still work. I consider this a bug because these files are at the wrong location now.
• If I change the path in the field setting and move the files, all old entries will be broken. I consider this a bug as well.

Correct behaviour in my eyes:

• If I change the path, all old entries should automatically be using this path.
• If I don't move the files, they will no longer be shown on the front-end because they are at the wrong location.
• If I move the files, everything will be fine.

[brendo]

Yeah, this doesn't make a lot of sense. We can always derive the path from the field setting. The only time this may be annoying is if you're querying entries out of context of a Field, in which case you'd have to do an additional lookup to find the setting (think XMLImporter). Minor though, and could be solved.

I wonder what we be involved in updating this. Out loud:

• Change appendFormattedElement to use the field's setting as a path
• Change saving to only save the filename, not the path in the data
• Fix up the import/export/parameter functions to also add the field settings path
• Double check the auto renaming/file exists logic so that it can handle just a filename

Doesn't seem too drastic.

[designermonkey]

It's not, and it's a good idea. Worries would include breaking other fields that extend the default Fiel Upload field.

Can those be checked before this is committed?

[michael-e]

The Unique Upload Field extension only deals with the filename, so I don't expect any problems here.

[nitriques]

I use a lot of @vlad-ghita's extension, mostly image_upload and multilingual_image_upload and I do not see anything getting broken. As long as the DS outputs the same values, it should be safe.

The only possible broken extension would be @andrewminton's enhanced_upload_field, since we did use the data duplicated in order to have a custom sub-folder for each files. This is making me think that maybe duplication IS a GOOD thing... I would hate to change the folder on a field and loose all my precious data that is already there... So I think we should:

1. Move the files to the new location ? This can take a long time....
2. Create aliases (symbolic link) to the new location ?
3. Leave this as-is ?

[brendo]

I had a look at the Enhance Upload Field and it looks like it uses $this->get('destination') rather than parsing the folders out from the filename, so I think this will be file too.

[nitriques]

Great!

[andrewminton]

Woop! tidy darts.

[brendo]

I've got this working locally, just curious how you'd like the migration to happen:

• On update to 2.3.3 automatically remove all file paths from all uploaded images that use the standard upload form
• On creation of new entries or the editing of existing entries, update the database to just save the filename. Update all various methods to handle both cases, filename will path, or just the filename.

[nilshoerrmann]

Personally, I'd prefer option 1 because it is straight forward and is not leaving me with two storage systems.

[designermonkey]

I agree, although it is more work in the migration file, it has a better benefit in the long run.

[brendo]

No worries, it's actually a lot simpler to do it in the migration than make the field tolerant :)

The downside is that fields that extend the upload field will not be catered for as they won't be in tbl_fields_upload... unless we extend this migration to also update some other known upload fields that we are sure won't break. This would be a first for us.

[michael-e]

extend this migration to also update some other known upload fields

Either that (which feels a bit strange) or inform the developers to use similar migration code in new versions of their extension. The updater could then say "If you are using upload field extensions, you must switch to compatible versions of these extensions."

Not sure though which solution I prefer, to be honest.

[nitriques]

Not sure though which solution I prefer, to be honest.

Me neither. I think the best way to do this would be with a wizard that guides the user through the steps...

[brendo]

a wizard

Very un Symphony like 👎

The updater could then say "If you are using upload field extensions, you must switch to compatible versions of these extensions."

We can definitely do this now we have the pre/post migration notes. God I love the installer in 2.3!

[nilshoerrmann]

… unless we extend this migration to also update some other known upload fields … This would be a first for us.

The core shouldn't interfere with extensions: running updates for extensions is an extension's task not a core one.
The developers should be notified before the change, the users just be notified during update.

[nitriques]

Looks ok to me.

[bernardodiasc]

@brendo I need a little help with this change.

With $this->get('related_field_id') (on displayPublishPanel) I can get the upload field ID, how can I get the file path of this field?

[bernardodiasc]

I think here isn't the place to question about that. I'll post this in the forum, as an issue related to the extension https://github.com/klaftertief/imagecropper (but also to the S2.3.3)

Searching about one hour for documentation, no success.

[bernardodiasc]

Here I am again, bring some important considerations about the path being saved with "/workspace". Is this intentional? Every image with JIT will break with this... Plese check this carefully.

About the questions above, already solved (http://www.getsymphony.com/discuss/thread/100199/).

[designermonkey]

I've never had a broken image with JIT when using Fiel Upload fields of any kind.

/workspace is the default file location folder in all of the upload fields, and in JIT, you just omit /workspace from your image path.

[bernardodiasc]

Yep, that's I'm talking. The "/workspace" are saving in database and returning in XML (@path) in Integration branch.

[bernardodiasc]

Here an example of what I'm saying:

<imagem size="1.18 MB" path="/workspace/uploads/destaques" type="image/jpeg">
    <filename>tostao1-517fdeb762b4f.jpg</filename>
    <meta creation="2013-04-30T12:09:43-03:00" width="1418" height="957" />
</imagem>

[designermonkey]

Ah, I see what you mean now. This will need fixing.

[brendo]

Fixed, thanks!

[bernardodiasc]

Thanks Brendan, working smooth now!

[nilshoerrmann]

The migration for this change doesn't work when updating a Symphony install that doesn't have any upload fields. I get the following error:

03 May 2013 08:24 > Notice: Updater - Migration to 2.3.2 was successful
Could not complete upgrading. MySQL returned: 1146: MySQL Error (1146): Table 'test.default_entries_data_10' doesn't exist in query: UPDATE default_entries_data_10 SET file = substring_index(file, '/', -1)
03 May 2013 08:24 > Notice: Updater - Migration to 2.3.3beta2 was unsuccessful

[nilshoerrmann]

Sorry, I was looking at the wrong table: there are upload fields in the install and the issue still exists.
There isn't a table with the name default_entries_data_10 though – there only is sym_entries_data_10 which represents an upload field indeed. Not sure where the default_ prefix comes from.

[nilshoerrmann]

Okay, here we go:

The migration file hardcodes the default_ prefix – it should use tbl_ instead.
https://github.com/symphonycms/symphony-2/blob/integration/install/migrations/2.3.3.php#L46

[brendo]

Fixed! Thanks, default_ is my table namespace in my local dev, so I must of copied this query directly from terminal into Textmate.

[nilshoerrmann]

Using the latest code in integration gives me the following error in the overview of sections containing an upload field:

Symphony Warning: array_key_exists() expects parameter 2 to be array, null given

An error occurred in /Users/nilshoerrmann/sites/webseiten/stil-bluete.net/symphony/lib/toolkit/fields/field.upload.php around line 210

205             $label->setAttribute('class', 'file');
206             if($this->get('required') != 'yes') $label->appendChild(new XMLElement('i', __('Optional')));
207
208             $span = new XMLElement('span', NULL, array('class' => 'frame'));
209
210             $filename = (array_key_exists('file', $data) && $data['file'])
211                 ? $this->get('destination') . '/' . basename($data['file'])
212                 : null;
213
214             if (array_key_exists('file', $data) && $data['file']) {

It doesn't matter if the upload field has content or not.

[nilshoerrmann]

$data is NULL in this case. I'm not sure why displayPublishPanel() is called on the indexes.

[nilshoerrmann]

It seems like Publish Filtering is the source of the problem. Still, the issue didn't exist before updating to the latest beta.

[nilshoerrmann]

This commit causes the issue: e631d85

[bernardodiasc]

@michael-e , are you sure?

The Unique Upload Field extension only deals with the filename, so I don't expect any problems here.

I check here, and filepath is saved on database.

Edit: The Unique Upload Field extend Upload Field, right? So maybe this is only a migration issue. When fetch tbl_fields_upload we should also fetch tbl_fields_uniqueupload.

[designermonkey]

As the field won't affect file paths from now on, then it doesn't need to run any updates. Yes there will be a problem with paths in existing tables, but the core won't and shouldn't upgrade extension tables.

You'll just have to do a manual change I'm afraid. Bit of a pain, and I've done it myself.

[bernardodiasc]

Understood @designermonkey , thanks for the explanation. I inject an update in the migration file to cover this ground and it is working now.

[designermonkey]

Yeah, that's what I did too :) Glad you got it sorted.

[michael-e]

I admit that I haven' added the update logic to the extension yet. I should do and release a new version.

[bernardodiasc]

@michael-e that's a very useful extension, would bee good if the unique option added in the core Upload Field. Is this possible?

[michael-e]

This has been proposed several times. I am not sure, however, if it is still on the list for Symphony 2. I don't think so, because we agreed on concentrating on bug fixes and "polishing".

My implementation (simply using a unique hash) has been a matter of discussion a while back. Some people said that they would prefer adding a "counter" to filenames that already exist (like the Unique Input Field can do it), because the hash part of the filename is ugly in their eyes. I don't like complicated solutions, however, and a unique hash (based on microtime) is the strongest solution and even allows moving files between folders without issues. So I wanted to keep the extension logic as it is.

[bernardodiasc]

Yeah, agreed. I don't think the user overriding by default her files is a good idea. I'm using Unique Upload field in all projects.