Issue #2830050 by bkosborne: FileUpload widget should allow file exte… #227
Conversation
…nsions from all supported media bundles.
| * @var \Drupal\Core\Entity\EntityStorageInterface | ||
| */ | ||
| protected $fieldStorage; | ||
|
|
There was a problem hiding this comment.
This is provided by the SourceFieldTrait, no longer needed
| $plugin_definition = $this->getPluginDefinition(); | ||
|
|
||
| $filter = function (MediaBundleInterface $bundle) use ($plugin_definition) { | ||
| $field = $this->getSourceField($bundle); | ||
| $field = $this->getSourceFieldForBundle($bundle); |
There was a problem hiding this comment.
I added this new method to the SourceFieldTrait and removed the locally defined method getSourceField below
|
This looks good to me. I'll give @phenaproxima a chance to look at it later in the week before merging since you said you have a workaround. Good catch. |
| @@ -13,6 +13,8 @@ | |||
| */ | |||
| abstract class BundleResolverBase extends PluginBase implements BundleResolverInterface, ContainerFactoryPluginInterface { | |||
|
|
|||
| use SourceFieldTrait; | |||
There was a problem hiding this comment.
I'm not sure about SourceFieldTrait being part of BundleResolverBase. It probably won't harm anything, but it isn't architecturally "pure", since the bundle resolver API shouldn't make assumptions about the media bundles in question.
There was a problem hiding this comment.
Wasn't it essentially already part of BundleResolverBase though? It had a method getSourceField which I removed in favor of using the trait since it was mostly code duplication.
There was a problem hiding this comment.
Good point. That's what I'm seeing too. Not sure how it got so hashed up.
| * @return MediaBundleInterface[] | ||
| * Applicable media bundles, keyed by ID. | ||
| */ | ||
| public function getPossibleBundles(); |
There was a problem hiding this comment.
Can we have a base implementation of this method in BundleResolverBase which simply returns all bundles, unconditionally?
There was a problem hiding this comment.
BundleResolverBase already has an implementation of this which I mostly kept untouched. Instead of returning all possible bundles it limits the bundles to those that have a matching source field definition.
| */ | ||
| protected function getSourceFieldForBundle(MediaBundleInterface $bundle) { | ||
| $type_config = $bundle->getType()->getConfiguration(); | ||
| if (!isset($type_config['source_field'])) { |
There was a problem hiding this comment.
Let's change this to if (empty()) so we can account for any falsy value.
| * The configurable source field entity. | ||
| */ | ||
| protected function getSourceFieldForBundle(MediaBundleInterface $bundle) { | ||
| $type_config = $bundle->getType()->getConfiguration(); |
There was a problem hiding this comment.
To re-use this code, let's change getSourceField() to:
return $this->getSourceFieldForBundle($entity->bundle->entity)
|
This is looking great. I have a couple of fairly minor requests, then I think this can be merged. |
…dle any falsey value for the source field name in SourceFieldTrait.
…nsions from all supported media bundles (#227)
This is to resolve this issue: https://www.drupal.org/node/2830050
This modified the
BundleResolverInterfaceto expose thegetPossibleBundles()method. I also refactored things a bit to make better use of theSourceFieldTrait. Not sure how comfortable you are with the API change. It seems like it would have little effect unless other users of Lightning decided to create their own bundle resolvers w/o extending the abstract one this module provides.As for testing, I'm unable to get any of the behat tests that require JS to run on my Mac unfortunately (I installed JDK, selenium standalone server, and the chromium driver, but I still get failure to connect errors when a javascript test tries to run =/)