Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added a Property manager. #207

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Added a Property manager. #207

wants to merge 1 commit into from

Conversation

techanon
Copy link
Contributor

Updated the file manager app to include a property manager as well as
adjusted the database schema to fit the requirements of the new format.

Updated the file manager app to include a property manager as well as
adjusted the database schema to fit the requirements of the new format.
@jbroadway
Copy link
Owner

I don't think a solution that uses alter table to add custom properties is the best approach. It create the potential for inconsistent database schemas between sites, which limits data portability especially for less technical users that may not know how to reconcile schema differences, or for keeping automated update and import/export tools simple and reliable.

The approach taken in other areas of Elefant was to use the ExtendedModel class, which stores extended properties in a single JSON-encoded field within the original database record itself (see users and blog posts). There are limitations such as lack of indexing on custom properties, but it means everything is fetched in the initial database call, and no schema changes are needed to extend the properties list.

Another solution I've thought about was adding an ExtendedModel-based "files" table with just filename and properties fields, since there are already simple user interface elements for integrating it, found here:

http://www.elefantcms.com/wiki/JavaScript-and-Client-Side-Helpers#custom-fields

I waited on that though because I'm limiting new additions to Elefant 2 to bug fixes and only very minor features at this point. I'm thinking for now that it might be best to simply add a couple more hard-coded properties (link, credits) and leave it until the next release to decide on a more flexible long-term solution. I think between those three (description, link and credits), it ought to cover the majority of people's needs in the meantime.

I should also mention that concatenating strings into SQL queries via select '.$prop.' from opens you up to SQL injection attacks. Whenever you concatenate an SQL query, you have to be sure that the variables are defined internally or from trusted sources, never from external sources like $_GET data.

I'm going to mark this as "deferred" for now, because the issues I mentioned are fixable, and it may make more sense than ExtendedModel as a solution once Elefant 2 is out the door.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants