Problem/Motivation

In #2928256: Users shouldn't be able to change the media source plugin after the media type is created a change is made to not allow the media source for a media type to be changed. The source field type and metadata of the media sources can be different, which could lead to issues.

However, when creating a new type it is very easy to select the wrong source. If you are not allowed to change it right after your first selection, this could be very frustrating.

Proposed resolution

Allow changing the media source of the media type only when:

  • The media type is still new and not saved so far.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

seanB created an issue. See original summary.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dagomar’s picture

I took a stab at this, but have not completely finished. The included patch will allow the change of the source field if the below conditions are all true:

  • Values in the field map are empty
  • The only custom field is the source field
  • There are no entities of this type

It also always allows the change of the source field if the media entity is new.

What is not included in this patch:

  • Removal of source field
  • Test coverage

I'm still posting my patch to be able to discuss and review.

seanB’s picture

  1. +++ b/core/modules/media/src/MediaTypeForm.php
    @@ -392,4 +395,89 @@ public function save(array $form, FormStateInterface $form_state) {
    +   *   True if source can be changed, false if not.
    

    Bool returns are mostly written as: 'Whether the source can be changed or not.'

  2. +++ b/core/modules/media/src/MediaTypeForm.php
    @@ -392,4 +395,89 @@ public function save(array $form, FormStateInterface $form_state) {
    +  private function isSourceChangeable() {
    ...
    +  private function entityCount() {
    ...
    +  private function hasOnlySourceField() {
    ...
    +  private function fieldMapEmpty() {
    

    This should be protected instead of private.

  3. +++ b/core/modules/media/src/MediaTypeForm.php
    @@ -392,4 +395,89 @@ public function save(array $form, FormStateInterface $form_state) {
    +    if (!$this->fieldMapEmpty()) {
    ...
    +    if (!$this->hasOnlySourceField()) {
    ...
    +    if ($this->entityCount() > 0) {
    

    I think we can just combine these?

    if (!$this->entity->isNew() && (!$this->fieldMapEmpty() || !$this->hasOnlySourceField() || $this->entityCount())) {
      return FALSE;
    }
    
seanB’s picture

  1. +++ b/core/modules/media/src/MediaTypeForm.php
    @@ -392,4 +395,89 @@ public function save(array $form, FormStateInterface $form_state) {
    +  private function hasOnlySourceField() {
    +    // Make sure this is an existing media entity.
    +    if (!$this->entity->isNew()) {
    

    You can rewrite this method the return directly for new entities:

    if ($this->entity->isNew()) {
      return TRUE;
    }
    
  2. +++ b/core/modules/media/src/MediaTypeForm.php
    @@ -392,4 +395,89 @@ public function save(array $form, FormStateInterface $form_state) {
    +      if (is_array($fields) && count($fields) == 1 && isset($fields[$source_field->getName()])) {
    

    Maybe we can unset the source field in the list of fields and just check count($fields). Also the is_array() could be replaced with just $fields.

    if ($fields && count($fields)) {
    
  3. +++ b/core/modules/media/src/MediaTypeForm.php
    @@ -392,4 +395,89 @@ public function save(array $form, FormStateInterface $form_state) {
    +    $fields = $this->entity->getFieldMap();
    

    The var contains the field map, not fields, so I guess the var should be named $field_map?

  4. +++ b/core/modules/media/src/MediaTypeForm.php
    @@ -392,4 +395,89 @@ public function save(array $form, FormStateInterface $form_state) {
    +    if (!$fields || (count(array_unique($fields)) === 1 && end($fields) === MediaSourceInterface::METADATA_FIELD_EMPTY)) {
    

    This is a little unclear. Maybe it's better to loop over the fieldmap and unset all MediaSourceInterface::METATDATA_FIELD_EMPTY values?

seanB’s picture

Status: Active » Needs work
jefuri’s picture

Status: Needs work » Needs review
FileSize
3.37 KB
3.56 KB

Ready for review again, process all comments from @seanB from #4 en #5.

borisson_’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/media/src/MediaTypeForm.php
    @@ -392,4 +395,87 @@ public function save(array $form, FormStateInterface $form_state) {
    +  protected function isSourceChangeable() {
    +
    +    if (!$this->entity->isNew() && (!$this->fieldMapEmpty() || !$this->hasOnlySourceField() || $this->entityCount())) {
    +      return FALSE;
    +    }
    +
    +    return TRUE;
    +  }
    

    This can be rewritten to be a oneliner.

    protected function isSourceChangeable() {
      return $this->entity->isNew && ($this->fieldMapEmpty() || $this->hasOnlySourceField() || $this->entityCount() > 0);
    }
    
  2. +++ b/core/modules/media/src/MediaTypeForm.php
    @@ -392,4 +395,87 @@ public function save(array $form, FormStateInterface $form_state) {
    +    // Make sure this is an existing media entity.
    +    if ($this->entity->isNew()) {
    +      return TRUE;
    +    }
    

    This comment doesn't agree with the code under it.

    I think we can remove the comment.

yogeshmpawar’s picture

Assigned: Unassigned » yogeshmpawar
yogeshmpawar’s picture

Changes done as per comment #8 & also added an interdiff.

Status: Needs review » Needs work

The last submitted patch, 10: enable_source_field-2928851-10.patch, failed testing. View results

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
5.6 KB
3.6 KB

Fixed #8 and started working on some tests.

Status: Needs review » Needs work

The last submitted patch, 12: 2928851-12.patch, failed testing. View results

daniel.bosen’s picture

I was testing this a bit. First of all, it needs #2557299: Any AJAX call disregards machine name verification when AJAX is used and leads to a fatal error merged, otherwise you run into problems when you try to switch to source whos configuration contains required fields. An example would be the remote video source.

Otherwise, it seems to work on new created media types. I am not sure though if it is a good idea to allow changes to already created types. While the conditions to allow it are good, I am not convinced, that it is worth the effort. Complex additions to the modules have to be done to replace the source field, the fields configuration, and the field mappings. I think it is OK, to say this can not be changed anymore. We do the same thing for example with views, if you selected the wrong base settings, you have to recreate the whole view. So this seems acceptable.

I would propose to simplify the isSourceChangeable() to just test if the media type is new and decide if modification of an existing type should be a follow up to this.

chr.fritsch’s picture

We discussed this with @marcoscano and @seanB and agreed that we want only to allow the changing of the source during media type creation. Once the media type is saved, changing the source should not be possible. This is already a step forward.

daniel.bosen’s picture

Simplified patch without the possibility to change created types

daniel.bosen’s picture

Status: Needs work » Needs review

The last submitted patch, 3: enable_source_field-2928851-3.patch, failed testing. View results

marcoscano’s picture

+++ b/core/modules/media/src/MediaTypeForm.php
@@ -118,7 +121,7 @@ public function form(array $form, FormStateInterface $form_state) {
-    if ($source) {
+    if (!$source_changeable) {

I agree with the approach, and it looks good to me. My only question is if we still need a method to check this, once we no longer have logic inside it. Couldn't we simplify and just use if (!$this->entity->isNew()) {, maybe with an inline comment to make it even more clear?

chr.fritsch’s picture

I had the same feeling when I first looked at the patch.

So here it is.

chr.fritsch’s picture

marcoscano’s picture

  1. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaTypeCreationTest.php
    @@ -11,6 +11,52 @@
    +
    

    Nit: Unnecessary newline

  2. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaTypeCreationTest.php
    @@ -11,6 +11,52 @@
    +    $this->assertSession()->assertWaitOnAjaxRequest();
    ...
    +    $this->assertSession()->assertWaitOnAjaxRequest();
    

    We can use $assert_session->assertWaitOnAjaxRequest() here.

    However, I think that is recommended to be used only as a last resort, waiting for a real element on the page is apparently more robust. I believe at least the first assert could be replaced

    https://www.drupal.org/node/2846936

  3. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaTypeCreationTest.php
    @@ -11,6 +11,52 @@
    +    // Create a media entity.
    +    $this->drupalGet("/media/add/$mediaTypeMachineName");
    +    $page->fillField('Name', 'foo');
    +    $page->fillField('Test source', 'test');
    +    $page->pressButton('Save');
    

    Do we still need this part of the test?

  4. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaTypeCreationTest.php
    @@ -11,6 +11,52 @@
    +    $assert_session->pageTextContains('The media source cannot be changed after the media type is created');
    

    Should we assert also that the field is disabled?

daniel.bosen’s picture

Status: Postponed » Needs review
Issue tags: +DevDaysLisbon
FileSize
3.5 KB
1.62 KB

Addressing all review items.

marcoscano’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me, once it gets green

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/media/src/MediaTypeForm.php
@@ -76,6 +76,9 @@ public function form(array $form, FormStateInterface $form_state) {
+    // The source field can only be changed for new media types.

@@ -118,7 +121,7 @@ public function form(array $form, FormStateInterface $form_state) {
-    if ($source) {
+    if (!$source_changeable) {

@@ -141,7 +144,7 @@ public function form(array $form, FormStateInterface $form_state) {
-      '#disabled' => !empty($source),
+      '#disabled' => !$source_changeable,

Let's not have a local variable. Everywhere else we do stuff like this we do $this->entity->isNew() everywhere we need it. For example in this form:

'#disabled' => !$this->entity->isNew(),

for the machine name.

Edited for clarity

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
3.08 KB
1.33 KB

I suppose you meant "Let's not have a local variable".

So, here is the patch

marcoscano’s picture

Status: Needs review » Reviewed & tested by the community

👍

phenaproxima’s picture

Issue tags: +Media Initiative

Tagging as a Media Initiative issue.

chr.fritsch’s picture

Issue summary: View changes

Updated the IS

phenaproxima’s picture

Title: Allow the media source of a media type to be changed » Allow the media source of a media type to be changed when creating a new media type

Slight re-title.

phenaproxima’s picture

Status: Reviewed & tested by the community » Needs work

Nice find, and nice fix -- the test just needs to be groomed a bit.

  1. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaTypeCreationTest.php
    @@ -11,6 +11,47 @@
    +    // Wait for machine name generation. Default: waitUntilVisible(), does not
    +    // work properly.
    +    $session->wait(5000, "jQuery('.machine-name-value').text() === '{$mediaTypeMachineName}'");
    

    Why doesn't it work properly? Can't we assert that the actual hidden field of the machine name has the expected value? (machine_name_hidden_field.value === media_type_machine_name)

  2. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaTypeCreationTest.php
    @@ -11,6 +11,47 @@
    +    $assert_session->fieldExists('Media source');
    +
    +    $assert_session->optionExists('Media source', 'test_different_displays');
    +    $page->selectFieldOption('Media source', 'test_different_displays');
    

    We can collapse these lines into one: $assert_session->selectExists('Media source')->selectOption('test_different_displays')

  3. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaTypeCreationTest.php
    @@ -11,6 +11,47 @@
    +    $assert_session->fieldExists('Media source');
    +    $assert_session->optionExists('Media source', 'test');
    +    $page->selectFieldOption('Media source', 'test');
    

    Same here.

  4. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaTypeCreationTest.php
    @@ -11,6 +11,47 @@
    +    $this->assertSession()->assertWaitOnAjaxRequest();
    

    Why not use $assert_session?

alexpott’s picture

  1. Sorry I missed some things. :(
  2. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaTypeCreationTest.php
    @@ -11,6 +11,47 @@
    +    // Wait for machine name generation. Default: waitUntilVisible(), does not
    +    // work properly.
    +    $session->wait(5000, "jQuery('.machine-name-value').text() === '{$mediaTypeMachineName}'");
    

    I think it does work properly now! Atleast this can be
    $assert_session->waitForElementVisible('css', '.machine-name-value');

  3. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaTypeCreationTest.php
    @@ -11,6 +11,47 @@
    +    $this->assertSession()->assertWaitOnAjaxRequest();
    

    Is there an option to wait for something here?

daniel.bosen’s picture

Status: Needs work » Needs review
FileSize
2.92 KB
1.94 KB

Addressed all of the issues, the replacement for $this->assertSession()->assertWaitOnAjaxRequest(); is a bit clunky, but the text is the only thing that actually changes.

seanB’s picture

Status: Needs review » Reviewed & tested by the community

I like the way this was simplified. Thank you!
Works as expected and I think everything is addressed.

alexpott’s picture

Crediting all the reviewers.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 1a8b7f1 and pushed to 8.6.x. Thanks!

  • alexpott committed 1a8b7f1 on 8.6.x
    Issue #2928851 by chr.fritsch, daniel.bosen, yogeshmpawar, jefuri,...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.