Use league/flysystem to abstract attachments

[glensc]

Move attachments out of the database #251

uses league/flysystem, requires php 5.5.9+

---

For upgrading from 3.2.3 -> 3.3.0 there should be no additional disk space required. The upgrade process just adds the new tables and columns but leaves existing attachments in the current table. New attachments will be inserted into the new table but unless you run migrate_storage_adapter.php it won't move anything.

[glensc]

mount manager: http://flysystem.thephpleague.com/mount-manager/ could came useful

[glensc]

may even decide to use caching: http://flysystem.thephpleague.com/caching/

[glensc]

so the idea is to abstract issue_attachment_file table access, issue_attachment can stay in database. you can see existing commits in this PR to see how flysystem is used.

• create src/Flysystem/Adapter/AttachmentStore.php using current database (better name suggestion welcome)
• use MountManager and map attachment:// to configured adapter
• use calls like $filesystem->has("attachment://$iaf_id"), $filesystem->put($path, $contents), $filesystem->read($path)

some questions arise:

• how do we want to organie the attachment files?
• $iat_id
• $prj_id/$iat_id/$iaf_id.$extension
• $prj_id/$iat_id/$usr_id-$iaf_id.$extension
• $prj_id/$iss_id/$iat_id/$usr_id-$iaf_id.$extension
• ...?

@balsdorf WDYT?

[glensc]

probably should just store attachments as $iaf_iat_id-$iaf_id-$iaf_filename because there isn't enough context information for anything else. altho could use subdirs. or leave the decision to user, i.e have it configurable?

mysql> desc issue_attachment_file;
+------------------+------------------+------+-----+---------------------+----------------+
| Field            | Type             | Null | Key | Default             | Extra          |
+------------------+------------------+------+-----+---------------------+----------------+
| iaf_id           | int(10) unsigned | NO   | PRI | NULL                | auto_increment |
| iaf_iat_id       | int(10) unsigned | NO   | MUL | 0                   |                |
| iaf_file         | longblob         | YES  |     | NULL                |                |
| iaf_filename     | varchar(255)     | NO   |     |                     |                |
| iaf_filetype     | varchar(255)     | YES  |     | NULL                |                |
| iaf_filesize     | varchar(32)      | NO   |     |                     |                |
| iaf_created_date | datetime         | NO   |     | 0000-00-00 00:00:00 |                |
+------------------+------------------+------+-----+---------------------+----------------+
7 rows in set (0.01 sec)

[glensc]

searched github for some existing adapters, mysql, pdo flysystem

• https://github.com/CrazyDevil/flysystem-mysql - GPL, 6 releases, 12 commits, looks incomplete, hardcoded table structure, creates connection itself with hardcoded utf8
• https://github.com/IntegralSoftware/flysystem-pdo-adapter - MIT, 2 releases, 5 commits, generic pdo adapter, looks usable, but also fixed db structure
• https://github.com/phlib/flysystem-pdo - LGPL, 3 releases, 43 commits, uses chunks, compression, table_prefix configurable, memory usage optimizations

looks to me phlib/flysystem-pdo is a win

[glensc]

phlib/flysystem-pdo changes past last release include drop php 5.5 support

we can perhaps also up min requirement to php 5.6, which leaves only 5.6, as 7.0 is not supported for eventum :)

it also includes change license to LGPL-3, that's still ok with eventum right?

[glensc]

phlib/flysystem-pdo seems usable. to simplify integration, should use it's provided table structure. to use for multiple purposes (besides attachments), we can play with "table_prefix". in this example i used default "flysystem", so that could be "attachment" for this use case.

mysql> select * from flysystem_path;
+---------+------+---------------------------+------------+------------+------+---------------+---------------------+
| path_id | type | path                      | mimetype   | visibility | size | is_compressed | update_ts           |
+---------+------+---------------------------+------------+------------+------+---------------+---------------------+
|       1 | file | testPhlibFlysystemPdo.txt | text/plain | public     |   23 |             0 | 2017-05-12 23:11:47 |
+---------+------+---------------------------+------------+------------+------+---------------+---------------------+
1 row in set (0.00 sec)

mysql> select * from flysystem_chunk;
+---------+----------+-------------------------+
| path_id | chunk_no | content                 |
+---------+----------+-------------------------+
|       1 |        0 | bite my shiny metal ass |
+---------+----------+-------------------------+
1 row in set (0.00 sec)

and must use $iaf_id or $iat_id in path to avoid collisions of filenames from different attachments.

[balsdorf]

I'm good with upping requirement to php 5.6.

License compatibility is a good question. Eventum is licenses as "GNU General Public License, version 2 or later (GPL-2+)". GPL2 is incompatible with LGPL3, but since it says "or later" you can deem it GPL3 which then means it is compatible with LGPL3. https://www.gnu.org/licenses/gpl-faq.html#AllCompatibility

[balsdorf]

The issue with switching to using this adapter (which looks nice) is that all our current attachments in the DB will have to be migrated to the new table, which could be a huge job.

[balsdorf]

For the filesystem, I like subdirectories and I'd use this:

• $iss_id/$iaf_id/$filename.$ext

EDIT: Removed $prj_id and info from path after doing some prototyping. $iss_id is redundant as well, but will help keep things organized.

[balsdorf]

So I created a "LegacyAdapter" to read from our current, attachment stored files. From messing around with this, I think we should use MountManager to support multiple adapters at the same time. For the most part, I think we would just use `legacy://' and either 'pdo://' or 'local://' but the setup file could be used to be specify additional adapters.

We would still use issue_attachment and issue_attachment_file with the addition of a iaf_flysystem_path field. We would continue to store meta data ourselves since we need to easily display it, but reading / writing the actual contents of the files would all be handled by flysystem.

Old files would be read from legacy:// but all new files would be written to the default adapter.

We could also provide a migrate command to move files from legacy to the default adapter, but for users with large databases this could be quite painful.

Thoughts?

[glensc]

@balsdorf rebased against master to resolve conflicts. go ahead and push your changes out. your plan is probably fine.

[balsdorf]

So I've just pushed my work on this. At first I tried to make minimal changes but wasn't happy with how messy it was so I pretty much redid all of the attachment system. Here are the key points.

New class Eventum\Attachment\StorageManager:
This class abstracts the details of Flysystem away and is responsible for loading the adapters from the config file. This can also be used if we want to store non-attachment files (not sure what yet, but maybe a logo for customization css?). I've thought that this maybe should be in Eventum\Storage\

New class Eventum\Attachment\AttachmentManager:
Handles most of the functionality in the old attachment class. Handles uploads, associates them with issues, lists attachments, etc.

New class Eventum\Attachment\Attachment:
Represents an individual attachment in the database. Saves / Loads meta data from issue_attachment_file.

New class Eventum\Attachment\AttachmentGroup:
Represents a group of attachments related to an issues. Maps to the issue_attachment table.

Adapter for old files in Eventum\Attachment\EventumLegacyAdapter:
Loads data from issue_attachment_file. Not capable of writing or updating. Possibly should be moved to Eventum\Storage\

Since I was re-writing this I also added the ability to specify a minimum role for AttachmentGroups instead of just public/private.

Currently, by default attachments are still stored in the database, just with the new PDO Adapter. I believe we should make this configurable in the admin and during installation. For the local adapter I used the subpath "storage/" but I'm open to better suggestions.

As far as paths go, when files are first uploaded (before they are associated with an issue) the path is "unassociated/$iaf_id-$filename". When they are associated they are moved to "$issue_id/$iaf_id-$filename".

TODO:

• Update workflow system for new Attachment objects
• Script to move from one backend to another
• Display nice error if attachment cannot be found in any matching backends.

[balsdorf]

I know it doesn't make sense, but getMimeType() and getTimeStamp() are supposed to return the whole array of meta data. At last that is what the LocalAdapter does.

The migrate script uses the mount manager to move the file. Internally, that code copies the file and once the copy is confirmed deletes the old file. The only risk would be if for some reason the path in eventum cannot be updated. I do believe this is a small risk though and would only impact one file before erroring out. Considering this is an advanced script I think that is an acceptable risk. However, I did find one small bug I will fix and will also add some warnings to it and a note about OPTIMIZE TABLE.

[balsdorf]

Thanks for those fixes BTW. I'll update the references to 5.6 in the code and update the wiki

[glensc]

some nitpicking i considered:

perhaps use touch to set filesystem file dates from database. could be useful for some cleanups or analyze tools that use file timestamps.

as i understood flysystem stores such meta in it's database, but be useful to duplicate this on actual files as well. applies to migrate script mostly, but may have effect when move is used elsewhere.

[glensc]

note to self:
this PR removes iat_status column (20170630200513_eventum_attachments_drop_status.php), so going back from this branch (say master), makes whole eventum spew errors.

alter table issue_attachment add
  `iat_status` enum('internal','public') NOT NULL DEFAULT 'public';

EDIT: added rollback support (20170630200118 is patch before EventumAttachmentsDropStatus):

$ ./vendor/bin/phinx rollback -d 20170630200118

➔ ./vendor/bin/phinx status
...
 Status  [Migration ID]  Started              Finished             Migration Name
----------------------------------------------------------------------------------
     up  20170427170945  2017-05-12 22:56:58  2017-05-12 22:56:58  EventumInitDatabase
     up  20170428180919  2017-05-12 22:56:58  2017-05-12 22:56:58  EventumInitialData
     up  20170512193942  2017-05-12 23:16:58  2017-05-12 23:16:58  EventumFlysystemPdo
     up  20170512210935  2017-05-20 22:53:45  2017-05-20 22:53:45  EventumExtensionClass
     up  20170512222549  2017-05-20 22:53:45  2017-05-20 22:53:45  EventumExtensionMigrateDb
     up  20170630191008  2017-07-19 23:27:22  2017-07-19 23:27:22  EventumAttachments
     up  20170630200118  2017-07-19 23:27:22  2017-07-19 23:27:23  EventumAttachmentsMigrate
   down  20170630200513                                            EventumAttachmentsDropStatus
   down  20170705144419                                            EventumMoveIssueHistory

[glensc]

@balsdorf i also wish you do the last 3.2 release, to see the validity of docs and maybe some ideas/issues you encounter! :)

[balsdorf]

Good idea on touching the file, I've incorporated that into my changes to migration script.

[glensc]

i probably want to relocate APP_PATH . '/var/storage/' -> /var/lib/eventum/storage when i deploy this using rpm package. should we setup dynamic value for this using Setup or I should just patch every match for direct use in codebase?

like for example: lib/eventum/class.setup.php:246

[balsdorf]

I was thinking about using a dynamic value from setup, but I didn't want someone to change it and think all the files would be moved automatically. However, I'll add that as it is better then you having to patch a bunch of files.

…

On Tue, Jul 25, 2017 at 12:54 AM Elan Ruusamäe ***@***.***> wrote: i probably want to relocate APP_PATH . '/var/storage/' -> /var/lib/eventum/storage when i deploy this using rpm package. should we setup dynamic value for this using Setup or I should just patch every match for direct use in codebase? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#254 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAHzdfzO1doLwot_xt7xofDZw5Slidrgks5sRYMygaJpZM4NQtTC> .

[balsdorf]

When going to implement a path in setup for you I realized that no changes were needed, you just need to specify your own adapter:

  'attachments' =>
  array (
    'default_adapter' => 'local',
    'adapters' => array(
        'local' =>  array(
            'class' =>  '\\League\\Flysystem\\Adapter\\Local',
            'options'   =>  array('/path/to/my/system/')
        ),
    ),
  ),

This will override the "default" local file location. The only item you would have to patch is in the migration script, and only if you want to "touch" the files

[glensc]

yay for merge! don't forget to update docs for 5.6 min requirement. thanks!

wish me good luck rebasing #263 (it contained also changes (cleanups)) to attachment area :(

[glensc]

@balsdorf found some error, the $mimetype is always uninitialized here:
lib/eventum/class.misc.php:740