Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#33 | interdiff-2928851-26-33.txt | 1.94 KB | daniel.bosen |
#33 | 2928851-33.patch | 2.92 KB | daniel.bosen |
#26 | interdiff-2928851-23-26.txt | 1.33 KB | chr.fritsch |
#26 | 2928851-26.patch | 3.08 KB | chr.fritsch |
#23 | interdiff-2928851-20-23.txt | 1.62 KB | daniel.bosen |
Comments
Comment #3
dagomar CreditAttribution: dagomar commentedI 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:
It also always allows the change of the source field if the media entity is new.
What is not included in this patch:
I'm still posting my patch to be able to discuss and review.
Comment #4
seanBBool returns are mostly written as: 'Whether the source can be changed or not.'
This should be protected instead of private.
I think we can just combine these?
Comment #5
seanBYou can rewrite this method the return directly for new entities:
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.
The var contains the field map, not fields, so I guess the var should be named
$field_map
?This is a little unclear. Maybe it's better to loop over the fieldmap and unset all
MediaSourceInterface::METATDATA_FIELD_EMPTY
values?Comment #6
seanBComment #7
jefuri CreditAttribution: jefuri as a volunteer and at Synetic commentedReady for review again, process all comments from @seanB from #4 en #5.
Comment #8
borisson_This can be rewritten to be a oneliner.
This comment doesn't agree with the code under it.
I think we can remove the comment.
Comment #9
yogeshmpawarComment #10
yogeshmpawarChanges done as per comment #8 & also added an interdiff.
Comment #12
chr.fritschFixed #8 and started working on some tests.
Comment #14
daniel.bosenI 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.
Comment #15
chr.fritschWe 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.
Comment #16
daniel.bosenSimplified patch without the possibility to change created types
Comment #17
daniel.bosenComment #19
marcoscanoI 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?Comment #20
chr.fritschI had the same feeling when I first looked at the patch.
So here it is.
Comment #21
chr.fritschWe should postpone this on #2557299: Any AJAX call disregards machine name verification when AJAX is used and leads to a fatal error
Comment #22
marcoscanoNit: Unnecessary newline
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
Do we still need this part of the test?
Should we assert also that the field is disabled?
Comment #23
daniel.bosenAddressing all review items.
Comment #24
marcoscanoLooks good to me, once it gets green
Comment #25
alexpottLet'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:for the machine name.
Edited for clarity
Comment #26
chr.fritschI suppose you meant "Let's not have a local variable".
So, here is the patch
Comment #27
marcoscano👍
Comment #28
phenaproximaTagging as a Media Initiative issue.
Comment #29
chr.fritschUpdated the IS
Comment #30
phenaproximaSlight re-title.
Comment #31
phenaproximaNice find, and nice fix -- the test just needs to be groomed a bit.
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)
We can collapse these lines into one:
$assert_session->selectExists('Media source')->selectOption('test_different_displays')
Same here.
Why not use $assert_session?
Comment #32
alexpottI think it does work properly now! Atleast this can be
$assert_session->waitForElementVisible('css', '.machine-name-value');
Is there an option to wait for something here?
Comment #33
daniel.bosenAddressed all of the issues, the replacement for
$this->assertSession()->assertWaitOnAjaxRequest();
is a bit clunky, but the text is the only thing that actually changes.Comment #34
seanBI like the way this was simplified. Thank you!
Works as expected and I think everything is addressed.
Comment #35
alexpottCrediting all the reviewers.
Comment #36
alexpottCommitted 1a8b7f1 and pushed to 8.6.x. Thanks!