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

Media multifile #756

Merged
merged 23 commits into from
Sep 17, 2020
Merged

Media multifile #756

merged 23 commits into from
Sep 17, 2020

Conversation

ajstanley
Copy link

@ajstanley ajstanley commented Jan 24, 2020

What does this Pull Request do?

Adds the ability to create derivative files to attach to existing media.

What's new?

A MediaSourceHasMimetype condition to allow media-level behaviours based on a Media's sourcefile's mimetype.
A new Reaction to generate files with microservices, and have them attach to existing media.
A new Action to define which queue the file is sent to, and where the returned derivative is to be attached.
A new action to both attach an extracted text file to an existing media, and populate an editable text field with the contents of that file.
A modification to islandora_entity_view_mode_alter to allow media views to be altered as well as node views.

How should this be tested?

With this patch in place all current Islandora Actions, Reactions and Contexts should continue to function as they have in the past.
A user should be able to add a file field to an existing media type, create an Action to populate that field, and create a context to cause that Action to fire.
Demo here. (Rough cut full of typos and stumbling, but you'll get the idea)

Additional Notes:

I'll put a small features module together over the course of the next few days to make a quick evaluation a little easier

Example:

  • Does this change the interface, add a new feature, or otherwise change behaviours that would require updating documentation?
  • Does this change add any new dependencies?
  • Does this change require any other modifications to be made to the repository (ie. Regeneration activity, etc.)?
  • Could this change impact execution of existing code?

Interested parties

@Islandora-Devops/committers

Copy link

@dannylamb dannylamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I generally agree with all this. There's some future improvement I'd like to see before releasing it, but I'm happy to bring this in sooner rather than later.

Other than the fact that this needs tests, I'd like to see us collapse those actions and use states to let folks choose between the three use cases we seem to have for derivatives

  • Make a new file and make a new media to hold that file.
  • Make a new file and reference it with a file field on an existing Media.
  • Don't make a file and instead put the file's contents on a text based field.

I'm fine with making follow up issues for those once we get this in. For now though, I'd say fix up coding standards and we'll iterate.

@mjordan
Copy link

mjordan commented Aug 2, 2020

Following along with @ajstanley 's video, I am getting a WSOD and the following error whenever I access Drupall:

[Sun Aug 02 18:13:20.978166 2020] [php7:notice] [pid 1585] [client 10.0.2.2:53296] Uncaught PHP Exception Drupal\\Component\\Plugin\\Exception\\PluginNotFoundException: "The "islandora_entity_bundle" plugin does not exist. Valid plugin IDs for Drupal\\Core\\Condition\\ConditionManager are: request_path_exclusion, entity_bundle, parent_node_has_term, node_had_namespace, node_is_published, node_has_term, media_uses_filesystem, media_has_mimetype, content_entity_type, media_is_islandora_media, node_has_parent, media_has_term, node_is_islandora_object, file_uses_filesystem, media_source_mimetype, language, node_type, request_path, current_theme, user_role" at /var/www/html/drupal/web/core/lib/Drupal/Component/Plugin/Discovery/DiscoveryTrait.php line 53, referer: http://localhost:8000/node/add/islandora_object

Strangely enough, I was able to select Islandora entity bundle when I created by reaction.

@mjordan
Copy link

mjordan commented Aug 4, 2020

Brand new vagrant, created using the ubuntu/bionic64 base box. I'm getting the same error as above after I switch to the media_multifile branch and rebuild my cache: Uncaught PHP Exception Drupal\\Component\\Plugin\\Exception\\PluginNotFoundException: "The "islandora_entity_bundle" plugin does not exist. Valid plugin IDs for Drupal\\Core\\Condition\\ConditionManager are: request_path_exclusion, entity_bundle, parent_node_has_term, node_had_namespace, node_is_published, node_has_term, media_uses_filesystem, media_has_mimetype, content_entity_type, media_is_islandora_media, node_has_parent, media_has_term, node_is_islandora_object, file_uses_filesystem, media_source_mimetype, language, node_type, request_path, current_theme, user_role" at /var/www/html/drupal/web/core/lib/Drupal/Component/Plugin/Discovery/DiscoveryTrait.php line 53.

@ajstanley
Copy link
Author

ajstanley commented Aug 4, 2020 via email

@mjordan
Copy link

mjordan commented Aug 4, 2020

I'm going to be away from my computer until next Tuesday so no huge rush on my end.

@ajstanley
Copy link
Author

I remerged 8.x-1.x to get rid of the islandora_entity_bundle errors.
Just tested and It works on my machine

@mjordan mjordan requested a review from dannylamb August 18, 2020 14:33
@mjordan
Copy link

mjordan commented Aug 18, 2020

@ajstanley tested with "Generate an Image Derivative for Media Attachment" action and works as described. Would be good to have some written docs on how to use this feature but we can do that later. Not sure if @dannylamb's review has been addressed, I'll turn it back to him.

@ajstanley
Copy link
Author

Thanks @mjordan.
I pulled the .install which added txt as an extension, so Danny's request is no longer relevant.  Travis doesn't like that most of AbstractGenerateDerivative and AbstractGenerateDerivativeMediaFile are the same.  I did that because Drupal gets super cranky when you try to change an extended Plugin's constructor signature, but I think I can get around that by using a setter injection in the create function.
Stay tuned for upcoming modifications.

@mjordan
Copy link

mjordan commented Aug 18, 2020

Also, just noticed that choosing the "Derive file For Existing Media" reaction category while creating a context is leaving warnings like these in the watchdog:

Warning: Cannot use a scalar value as an array in Drupal\Core\Render\Element\RenderElement::preRenderAjaxForm() (line 316 of /var/www/html/drupal/web/core/lib/Drupal/Core/Render/Element/RenderElement.php)

#0 /var/www/html/drupal/web/core/includes/bootstrap.inc(600): _drupal_error_handler_real(2, 'Cannot use a sc...', '/var/www/html/d...', 316, Array)
#1 /var/www/html/drupal/web/core/lib/Drupal/Core/Render/Element/RenderElement.php(316): _drupal_error_handler(2, 'Cannot use a sc...', '/var/www/html/d...', 316, Array)
#2 /var/www/html/drupal/web/core/lib/Drupal/Core/Render/Element/Link.php(88): Drupal\Core\Render\Element\RenderElement::preRenderAjaxForm(Array)
#3 [internal function]: Drupal\Core\Render\Element\Link::preRenderLink(Array)
#4 /var/www/html/drupal/web/core/lib/Drupal/Core/Security/DoTrustedCallbackTrait.php(100): call_user_func_array(Array, Array)
#5 /var/www/html/drupal/web/core/lib/Drupal/Core/Render/Renderer.php(781): Drupal\Core\Render\Renderer->doTrustedCallback(Array, Array, 'Render #pre_ren...', 'silenced_deprec...', 'Drupal\\Core\\Ren...')
#6 /var/www/html/drupal/web/core/lib/Drupal/Core/Render/Renderer.php(372): Drupal\Core\Render\Renderer->doCallback('#pre_render', Array, Array)
#7 /var/www/html/drupal/web/core/lib/Drupal/Core/Render/Renderer.php(200): Drupal\Core\Render\Renderer->doRender(Array, false)
#8 /var/www/html/drupal/web/core/lib/Drupal/Core/Template/TwigExtension.php(501): Drupal\Core\Render\Renderer->render(Array)
#9 /var/www/html/drupal/web/sites/default/files/php/twig/5f3bec730fe45_table.html.twig_jqxmmfA-W-bA9DlWKKJ8-q2iN/9rNi0Yn8jevAT56Z0GgoM63o4rX8X8U6Sdr5IRNd_R0.php(195): Drupal\Core\Template\TwigExtension->escapeFilter(Object(Drupal\Core\Template\TwigEnvironment), Array, 'html', NULL, true)
#10 /var/www/html/drupal/vendor/twig/twig/src/Template.php(453): __TwigTemplate_9afae8ffcc5b7e261b8630450c575f1f386fb0612a06ddba3486e81002e0c521->doDisplay(Array, Array)
#11 /var/www/html/drupal/vendor/twig/twig/src/Template.php(420): Twig\Template->displayWithErrorHandling(Array, Array)
#12 /var/www/html/drupal/vendor/twig/twig/src/Template.php(432): Twig\Template->display(Array)
#13 /var/www/html/drupal/web/core/themes/engines/twig/twig.engine(64): Twig\Template->render(Array)
#14 /var/www/html/drupal/web/core/lib/Drupal/Core/Theme/ThemeManager.php(384): twig_render_template('core/themes/sev...', Array)
#15 /var/www/html/drupal/web/core/lib/Drupal/Core/Render/Renderer.php(431): Drupal\Core\Theme\ThemeManager->render('table', Array)
#16 /var/www/html/drupal/web/core/lib/Drupal/Core/Render/Renderer.php(444): Drupal\Core\Render\Renderer->doRender(Array)
#17 /var/www/html/drupal/web/core/lib/Drupal/Core/Render/Renderer.php(200): Drupal\Core\Render\Renderer->doRender(Array, true)
#18 /var/www/html/drupal/web/core/lib/Drupal/Core/Render/Renderer.php(144): Drupal\Core\Render\Renderer->render(Array, true)
#19 /var/www/html/drupal/web/core/lib/Drupal/Core/Render/Renderer.php(573): Drupal\Core\Render\Renderer->Drupal\Core\Render\{closure}()
#20 /var/www/html/drupal/web/core/lib/Drupal/Core/Render/Renderer.php(145): Drupal\Core\Render\Renderer->executeInRenderContext(Object(Drupal\Core\Render\RenderContext), Object(Closure))
#21 /var/www/html/drupal/web/core/lib/Drupal/Core/Render/MainContent/ModalRenderer.php(22): Drupal\Core\Render\Renderer->renderRoot(Array)
#22 /var/www/html/drupal/web/core/lib/Drupal/Core/EventSubscriber/MainContentViewSubscriber.php(90): Drupal\Core\Render\MainContent\ModalRenderer->renderResponse(Array, Object(Symfony\Component\HttpFoundation\Request), Object(Drupal\Core\Routing\CurrentRouteMatch))
#23 [internal function]: Drupal\Core\EventSubscriber\MainContentViewSubscriber->onViewRenderArray(Object(Symfony\Component\HttpKernel\Event\GetResponseForControllerResultEvent), 'kernel.view', Object(Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher))
#24 /var/www/html/drupal/web/core/lib/Drupal/Component/EventDispatcher/ContainerAwareEventDispatcher.php(111): call_user_func(Array, Object(Symfony\Component\HttpKernel\Event\GetResponseForControllerResultEvent), 'kernel.view', Object(Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher))
#25 /var/www/html/drupal/vendor/symfony/http-kernel/HttpKernel.php(156): Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch('kernel.view', Object(Symfony\Component\HttpKernel\Event\GetResponseForControllerResultEvent))
#26 /var/www/html/drupal/vendor/symfony/http-kernel/HttpKernel.php(68): Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object(Symfony\Component\HttpFoundation\Request), 1)
#27 /var/www/html/drupal/web/core/lib/Drupal/Core/StackMiddleware/Session.php(57): Symfony\Component\HttpKernel\HttpKernel->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#28 /var/www/html/drupal/web/core/lib/Drupal/Core/StackMiddleware/KernelPreHandle.php(47): Drupal\Core\StackMiddleware\Session->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#29 /var/www/html/drupal/web/core/modules/page_cache/src/StackMiddleware/PageCache.php(106): Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#30 /var/www/html/drupal/web/core/modules/page_cache/src/StackMiddleware/PageCache.php(85): Drupal\page_cache\StackMiddleware\PageCache->pass(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#31 /var/www/html/drupal/web/core/lib/Drupal/Core/StackMiddleware/ReverseProxyMiddleware.php(47): Drupal\page_cache\StackMiddleware\PageCache->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#32 /var/www/html/drupal/web/core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php(52): Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#33 /var/www/html/drupal/vendor/stack/builder/src/Stack/StackedHttpKernel.php(23): Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#34 /var/www/html/drupal/web/core/lib/Drupal/Core/DrupalKernel.php(708): Stack\StackedHttpKernel->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#35 /var/www/html/drupal/web/index.php(19): Drupal\Core\DrupalKernel->handle(Object(Symfony\Component\HttpFoundation\Request))
#36 {main}

@ajstanley
Copy link
Author

@mjordan The scalar #ajax error seems to be unrelated -
It shows up as soon as you try to select any reaction. I tried the suggested cure of deleting line 243 of ContextUIController.php and it worked well.

Other than spamming the logs it seems pretty harmless.

@mjordan
Copy link

mjordan commented Aug 18, 2020

Thanks for looking into that warning - I tried to narrow it down to your branch but I guess I didn't do a good enough job. Sorry for the flase alarm.

@dannylamb
Copy link

Giving this another go. If it works good I'm happy to pull it in and iterate further.

@dannylamb
Copy link

I've managed to get follow the steps and set up a media type and context to take advantage of this. Works as advertised. We'll need to iterate on this but it's good to get this in now so we can start taking advantage of it.

I'm not super sure what's going on with Travis, but he appears a bit stuck. Also Github is still convinced there's a conflict. I bet if you can get that sorted out it'll kick Travis back into gear @ajstanley

@ajstanley
Copy link
Author

ajstanley commented Sep 16, 2020 via email

@ajstanley
Copy link
Author

This build is now failing on coder tests a: Passed a week ago and nothing has changed, and b: Involve files I haven't touched.

@dannylamb
Copy link

Sniffs have changed under our feet in Travis. I just did it locally and everything passes coding standards.

I'm loathe to let this sit any longer. I'll set up another ticket to deal with the travis issue or bite the bullet and update the sniffs.

Let's get this in. Looks like I might have to close this one out via the command line to deal with the conflict.

@dannylamb
Copy link

Oh... and now it's no longer complaining about the conflict? 🤷‍♂️

}
}
}
return $this->isNegated();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After some digging I was able to find out why my content blocks stopped appearing, was a bit tricky as the cache had mislead me to think it was other problems. Checks for isNegated should not be used in ConditionPlugins as it causes a double negation, since the ConditionManager does the negation.
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Condition%21ConditionManager.php/8.9.x

public function execute(ExecutableInterface $condition) {
    if ($condition instanceof ConditionInterface) {
      $result = $condition
        ->evaluate();
      return $condition
        ->isNegated() ? !$result : $result;
    }
    throw new ExecutableException("This manager object can only execute condition plugins");
  }

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, nice catch @nigelgbanks

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

Successfully merging this pull request may close these issues.

4 participants