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

FIX VersionedFileUploadField::create() conflicts with other modules #42

Merged
merged 1 commit into from Oct 23, 2014

Conversation

micmania1
Copy link
Contributor

Its very unlikely that this specific field will need to be replaced anyway.

Its very unlikely that this specific field will need to be replaced anyway.
@micmania1
Copy link
Contributor Author

This is to fix a conflict between this module and the Select Upload module.

The issue currently is that select upload replaces all UploadFields using the following:

Injector:
    UploadField: SelectUploadField

This means that when this module tries to create a new VersionedFileUploadField, its actually creating a SelectUploadField which doesn't contain any of the required functionality.

Using new ensures that the correct class gets used.

@nyeholt
Copy link
Contributor

nyeholt commented Oct 23, 2014

Hm, very interesting. I'll accept the PR for now, I think there might some merit in having something like

Injector:
  VersionedFileUploader: 
    class: VersionedFileUploadField
    type: prototype

and using it like

$uploadField = $this->injector->create('VersionedFileUploader', 'ReplacementFile', '');

to allow people to override it if they like, but I think that until that usecase comes up, I'm happy to
leave it as a 'new' call for now.

nyeholt added a commit that referenced this pull request Oct 23, 2014
FIX VersionedFileUploadField::create() conflicts with other modules
@nyeholt nyeholt merged commit c4181d7 into symbiote:master Oct 23, 2014
@micmania1
Copy link
Contributor Author

Thanks for merging - I agree that its not the best solution. If anything I think its highlighted a weakness in the Injector API.

@tractorcow
Copy link

Hm, that is quite a good point.

Edit: Yeah you've already done that lol.

@nyeholt
Copy link
Contributor

nyeholt commented Oct 24, 2014

If anything I think its highlighted a weakness in the Injector API.

Sort of - turns out it's a side effect of the way configuration is looked up when you ask the injector to create an object, but there's no specific config defined for that object type. In SilverStripeServiceConfigurationLocator, when looking for config, it will look for config for parent class types if what you've called create for is a type, and no specific config exists for it. (I don't recall the exact use case, but it was needed for a few core things when first swapping things to using the injector).

So swapping to use what I posted earlier, where the injector is used to alias a "Named" item to a specific implementation a la $injector->create('NamedItem') instead of just using the class name would fix the issue; alternatively, having the explicit config of

Injector:
VersionedFileUploadField: VersionedFileUploadField

or, more verbosely

Injector
VersionedFileUploadField:
class: VersionedFileUploadField

would also fix the problem, because it means the injector would find explicit config for the class immediately, before looking up its parent classes for configuration.

@micmania1 micmania1 deleted the patch-1 branch October 27, 2014 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants