jchristopher / attachments

[WordPress Plugin] Attachments allows you to simply append any number of items from your WordPress Media Library to Posts, Pages, and Custom Post Types
wordpress.org/extend/plugins/attachments/
GNU General Public License v2.0
241 stars 78 forks source link

Fix the way field classes are loaded so that it is compatible with HHVM #151

Closed EdHurtig closed 6 years ago

EdHurtig commented 9 years ago

See #146

Basically it boils down to the fact that we shouldn't rely on get_declared_classes to actually be ordered chronologically by inclusion time. It is kinda fragile and unfortunately it is not how HHVM has implemented get_declared_classes.

Instead, I have simply stored the list of classes pre-inclusion then diffed it with the list of classes after inclusion to see what is new. array_diff

Because $field_types[ $type ] can only contain 1 class I have discarded any other classes that might have been added and opted to just use the first one in the list $new_classes[0]. There should not be a case where a file declared multiple classes AFAIK

Would be great to get this in for the next release so that I don't have to keep applying this patch every time I update the plugin

steffenmllr commented 8 years ago

@jchristopher Any change this get's merged - ran into the same error just now

EdHurtig commented 8 years ago

@jchristopher Honestly I feel like this entire function should just get simplified... there is no reason to use get_declared_classes() as far as I can tell, and it just feels wrong especially because $field_types is completely static, no way that it can be anything different from what it is declared to be:

$field_types = array(
    'text'      => ATTACHMENTS_DIR . 'classes/fields/class.field.text.php',
    'textarea'  => ATTACHMENTS_DIR . 'classes/fields/class.field.textarea.php',
    'select'    => ATTACHMENTS_DIR . 'classes/fields/class.field.select.php',
    'wysiwyg'   => ATTACHMENTS_DIR . 'classes/fields/class.field.wysiwyg.php',
);
jchristopher commented 8 years ago

@EdHurtig yes I agree, at a time the fields were going to be extended to be customizable https://github.com/jchristopher/attachments/blob/3.5.6/classes/class.attachments.php#L996-L997 but that has never made it out in the wild. I think hard-coding it will be just fine.

EdHurtig commented 8 years ago

@jchristopher Just simplified the function drastically. need to test it out first though

jchristopher commented 6 years ago

I realize this went un-merged for a long time and is likely no longer applicable, but given the announcement from HHVM that HHVM will not target PHP7 I'm going to close this PR.