Problem/Motivation
#2831940: Create file field widget on top of media entity is a big issue that's underway to support uploading files to an entity reference field for Media, and currently a blocker for the module to be exposed to users in the UI.
Meanwhile, though, media fields seem bewildering and broken to users who have no clear way to upload or find media they want to put in the media field. (It's an autocomplete field where you can type a keyword to locate the media you want, if the media has already been uploaded.) I think it could be beneficial to provide users some help on how to use the field, even when the module is still hidden.
Proposed resolution
- Help users see that two actions are possible, either submitting new media or using existing media.
- Provide links to the pages where they can actually list media and where they can search for it.
- Use
target="_blank"
to prevent users from losing work by clicking on the link. This isn't nearly as helpful as #1510544: Allow to preview content in an actual live environment, but it will at least prevent disasters. - Since
target="_blank"
is discouraged from an accessibility perspective, mitigate this by indicating in the text that a new window will be opened. - Only show the message about creating new media if the user has access to do so for the types that the field supports.
- Alter the widget form elements directly and provide a template for the help text so that it can be overridden by sites, themers, or other modules. (This was a recommendation from the usability review.
- We do not use it as preconfigured options because it needs to be translatable (which a default description would not be). It's similar to the need in #2421001: Fix regression in the link widget where help text does not show: we want to add supplemental information to the users' configured data.
Remaining tasks
- #2831940: Create file field widget on top of media entity is still a requirement; this is just an interim fix.
- The proposed has product and usability maintainer signoff (again, only as an interim fix).
- #2940201: hook_field_widget_form_alter() can no longer affect the whole widget for multi-value fields is a blocker.
User interface changes
Before
The user has no idea what to do with the field unless they're used to entity reference autocompletes.
To address this, we give the user some guidance on their other options, with links (that open in new windows) to the relevant places that will help with those other tasks.
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#123 | interdiff.txt | 1.89 KB | effulgentsia |
#116 | media-2936293-117.patch | 21.8 KB | xjm |
#114 | media-interdiff-114.txt | 4.21 KB | xjm |
#114 | media-2936293-114.patch | 23.54 KB | xjm |
#114 | media-2936293-114-FAIL.patch | 23.41 KB | xjm |
Comments
Comment #2
xjmComment #3
xjmComment #5
xjmJust a merge conflict with a commit in HEAD this morning; fixed here.
Comment #6
xjmComment #7
xjmComment #9
xjmThanks, testbot, for checking the syntax of my documentation example.
Comment #10
tedbowWhy not open this link in the off-canvas dialog instead? The dialog itself is no longer experimental.
They would remain in context.
Comment #12
cilefen CreditAttribution: cilefen at Institute for Advanced Study commentedComment #13
xjmRegarding #10, that sounds more like a proposal for the main issue in #2831940: Create file field widget on top of media entity (and I think the Media Library would benefit from using offcanvas). It probably has some of the same pros and cons as using a modal. For this issue though I'm hoping to get something hotfix-ish in that will help a little sooner rather than later, since the widget issue is both difficult and risky.
Comment #14
webchickTagging for UX team review.
Comment #15
Gábor HojtsyThe before/after screenshots only show the single form case, not the multivalue form case. Would be nice to see that. Comments on the text/behavior:
I would not add "media" on the end of this. We should look at which media type names would need that tacked on to be meaningful. The one screenshot definitely does not need it IMHO, it looks distracting and computer generated.
Can we filter for the right media type(s)? Otherwise people need to figure out there how to do it.
Comment #16
marcoscanoI love this issue!
IMHO, the
target="_blank"
is a very reasonable tradeoff, and justifiable in the context of the hotfix this is intended to address.In this patch I have:
- Found that the new hook was only being invoked for cardinality -1, so other multiple fields were not getting the text. Tried to fix it, it appears to work now for me.
- Tweaked a bit the hook documentation in field.api.php
- Fixed some minor CS issues
- Found that the new text can get be confusing if the previous field also has a multiple-line description. Maybe we could wrap everything inside a fieldset container to make more obvious that all this text is from a single field?
- Addressed #15.1
- About #15.2:
Strictly speaking, the site could have disabled views, and in that case the collection route doesn't have filters. We could add some logic that sends to different links depending if Views is installed or not, but maybe that would be overkill? IMHO I believe it is safe to assume at this point that most users know how to use exposed filters? (or I may have completely misunderstood your point, please correct me if so).
Some screenshots:
Patch in #9:
Patch in #9 but invoking the hook also for non-unlimited cardinality fields:
Adding wrappers and addressing #15.1:
Still needs tests, I can work on that if we all agree on how it should look like.
Comment #17
seanBThis seems like a really good idea! Have a bunch of nits and suggestions to change some interface texts.
Besides that it at least needs tests for the new hook and making sure the description is shown for media entity reference field (and not shown for other entity reference auto complete fields), but I see it's already tagged accordingly.
Leaving to "needs review" for now to get more opinions.
I'm kind of surprised about this. We should probably add documentation to
hook_field_widget_form_alter()
to show that this hook exists as well.Typo 'itesm'
This sentence feels a little weird to me? Maybe it's because English is not my first language.
Could we change it to:
Add a description to inform users where they can find and add new media. Only add the description on autocomplete widgets for entity reference field pointing to media.
Feel free to ignore this if it doesn't make any sense.
We can remove this blank line I think.
Again, English is not my first language but shouldn't "Type part" be "Type a part"
Can we show the message about the media list only once as well? Repeating that seems just as "spammy". Maybe something like:
Type a part of the media name. See the media list (opens a new window) to help locate media. Create new media via the media add page (opens a new window), then add it by name to the field below.
I proposed some small changes above. Mainly "upload" is not always applicable (for remote video for example). The link is to the media add page and not a media form, so that could be changed as well to avoid confusion.
Comment #18
xjmThanks @seanB, @marcoscano, and @Gábor Hojtsy! All good points.
For #17.1, the patch is also adding an
@see
to the other hooks, unless I've misunderstood.For #17.5, "Type part of" and "Type a part of" can be used interchangeably. I'd suggest sticking with the shorter version even if it's only one character shorter. :)
Comment #19
xjmI think this could be a followup issue since Views filters are fairly intuitive. However, what we might want to do is add Allowed media types: foo, bar to mitigate the "WTF I uploaded my 'Kitten video' media; where is it?" if the field only accepts 'Puppy video' media.
Comment #20
xjmIt might also be possible to do this by modifying the other hook in a BC way rather than adding API, but let's see what the UX team says before we spend time on that. :) (Ditto on tests.)
Comment #21
ckrinaDiscussed in the UX calls and it looked fine for us as an MVP. The
target="_blank"
might not be the best solution, but the help text makes it good enough for MVP.@roland.molnar mentioned that maybe some more text explaining how to extend the basic functionalities could help. One possible location might be Drupal's help.
Also it was suggested to add some more text in the description of the module, right now is:
'Create reusable media.'
. But maybe it can be a followup issue.Comment #22
DyanneNovaI think this makes sense for an MVP. I agree that target="_blank" isn't ideal, and that a Settings Tray dialogue would be better, but think this works in the interim.
My one nitpick is that I think "Upload new Photos and videos media" makes more sense than "Uploading new Photos and videos media" to match the wording of "Use existing Photos and videos media" vs "Using existing Photos and videos media"
Comment #23
tstoecklerAdding this new hook is a great idea. I cannot overstate how excited I am about this idea. It is incredibly useful for very many different use-cases and a lot of new room for experimentation in contrib. Let's add this in a separate issue, though, and get a sign-off from one of the Field API maintainers (
=== ['@amateescu']
) please.Comment #24
webchickWe reviewed this on the weekly Drupal Product Management call, and though there was much hand-wringing and gnashing of teeth, agreed with this as a stop-gap for the current behaviour to unblock exposing the module in the UI, since it is easily 3,000% better than what's in HEAD right now. (Though UX-wise, still not remotely comparable to even our most basic competitors out there, so we definitely have more work to do. :\)
Comment #25
marcoscanoNot sure if I covered all feedback, but this patch should address #17, #19, #22 and adds some tests.
Did not investigate the possibility of skipping the new hook (#20) because I feel there is some interest also outside of media to have it available? (based on #23)
Also, I think I would question the perception that the description text below each autocomplete element is spammy... Users tend not to read stuff. However, when they are trying to type something they are not sure what it is, it will be *certainly* read. Actually, I have the feeling that this is probably the most important piece of text that we are adding in this patch, so I would try to leave it there, even at the price of being repetitive if the field is multi-valued.
Please let me know if I missed something!
Comment #26
ershov.andrey CreditAttribution: ershov.andrey as a volunteer and at Cheppers commentedWhile testing patch from #25 I found that each time you add new item to the multi-value media field there is another fieldset added.
So I've moved the fieldset creation code to the top of the alter function and now it's working as expected.
Comment #27
xjm"Media multiple" indeed. :) Nice work @ershov.andrey!
Comment #28
xjmPromoting to major since this is now essentially the last blocker to make the core media module available to non-expert site builders.
Comment #29
xjmThe most recent patch seems to have introduced:
(That was with articles, cardinality 3; otherwise just Standard and Media out of the box.)
I'm working through debugging that now. Probably should also add to the tests for that. ;)
@tstoeckler and @marcoscano regarding the hook:
hook_field_widget_form_alter()
andhook_field_widget_WIDGET_TYPE_form_alter()
already exist; they were added in #1204230: Missing hook_field_widget_form_alter(). I know this because that was my very first core issue. :) When it was added it could alter both single- and multi-value elements. However, since 8.0.0 shipped like this, we need to provide BC for the current behavior. That's why I started by hacking together a new hook. (So there are four of them here, rather than two.)Agreed on the separate issue scope though for fixing that; I just included it here for the POC and didn't want to do additional work until the UX and Product teams had signed off.
Comment #30
xjm#2940201: hook_field_widget_form_alter() can no longer affect the whole widget for multi-value fields is that blocking issue.
Comment #31
xjmNot actually debugging at the moment in case someone else wants to pick it up.
The code can probably be cleaned up a bit. We should also probably actually create a proper Twig template rather than concatenating a bunch of markup into
Markup
objects. (I didn't add this in the original patch because I just wanted to hack together something for UX review/testing.) Actually, adding the template will clean up the code in and of itself so try that first. :)Comment #32
xjmAh and we also need a test to verify #26 does not happen.
Comment #33
xjmComment #34
xjmComment #35
xjmOh, forgot to mention that the notice in #29 is on the field add page in the Field UI (when it renders the widget for default values), not the node edit form.
Aside: adding a template will also address another concern that came up in the
MediaUX meeting, that they would not show this help text to a client and would instead use applicable contrib. If we add a template rather than hardcoding all this, modules/themes/sites can override it however they like.Comment #36
xjmOh, this got its UX review already. Thanks reviewers!
Comment #38
xjm@amateescu reviewed #2940201: hook_field_widget_form_alter() can no longer affect the whole widget for multi-value fields, which inadvertently included part or all of this issue. Oops. Here are the relevant points from his review that are not about the hook:
Also crediting him for the review that's applicable to this issue.
Comment #39
xjmI'm working on the template implementation (or trying to) and also testing out a modified approach based on feedback in #2940201: hook_field_widget_form_alter() can no longer affect the whole widget for multi-value fields.
Comment #40
xjmHere is a (broken) patch that starts putting together a template. I copied the template from
core/themes/classy/templates/form/field-multiple-value-form.html.twig
, but it seems to be missing some of the variables that has, so it renders everything except the actual widget. I couldn't figure out how to add them without creating infinite recursion. :P Maybe I should be adding a theme suggestion or something?Comment #41
tedbowOk looked at the example the system block and what it does in
system_theme
Ad then it adds the extra stuff it needs in
system_preprocess_block()
This seems to be what we need to do.
So I added
media_preprocess_media_reference_help
and added the are extra info there.It seems to work.
Comment #42
tedbowComment #45
xjmThanks @tedbow!
Working on fixing the last bits now.
Comment #46
xjmOkay, the attached patch finishes wiring this back up with a template instead of the wild, tangled jungle of render array alteration. :) We do still want to alter the actual element title from within our alter hook so that it appears in the correct place on the element's form. Next steps are:
Add back the access checking
Round out the tests.
Look into #38 more.
Finish tests for the blocker and get that in first so this patch can be rerolled on top of that.
Interdiff also adds the cardinality context from the latest direction in #2940201: hook_field_widget_form_alter() can no longer affect the whole widget for multi-value fields, which was not included yet in the recent patches here. Edit: fixed a wrong issue link.
Comment #47
xjmComment #49
xjmI'm cleaning the test up a bit to use a data provider.
Comment #50
phenaproximaThis should fix the tests. The assertions were quite out-of-date and looking for elements that didn't exist. Also DRYed things up a bit.
And here's a screenshot of the test running in Chrome:
EDIT: I'd been working on this for about an hour before @xjm posted her comment, so hopefully this doesn't step on anyone's toes!
Comment #51
xjmSo on #50 I'd already been working on my version for a few hours, although we did make a couple similar changes. Here's what I did:
One other test that could be added is testing the button to add another element, which we could add as only one test in the JS test. Not necessarily blocking though.
Interdiff is against my earlier patch (sorry, I was working on this on and off all day).
Comment #52
xjmI'll work a little more on cleaning the test up, inspired by @phenaproxima's patch.
Comment #53
davidhernandezThese spaces don't need to be here.
Lies and damn lies.
So, maybe it's just me, but something doesn't smell right. Do we ever put this much text in a template in core?
Comment #54
davidhernandezI haven't applied this and gone through all the comments, but are we trying to make this consistent with the core fieldset template? If so, the fieldset element should have an attributes object and the label should have a wrapping span for the label text.
Comment #55
phenaproximaThis seems pretty damn good to me. The tests are quite thorough so I'm removing the tag.
Nit: $cardinality should be outdented by two spaces.
I don't understand what this does, so I assume it's necessary...
Nit: The final two conditions are surrounded by superfluous parentheses.
Would it make more sense for $label to be the plural label of the target entity type?
$variables should be type hinted.
We don't need to pass \Drupal::currentUser() to the createAccess() call.
Comment #56
xjmcore/themes/classy/templates/form/fieldset.html.twig
and
template_preprocess_fieldset()
incore/includes/form.inc
We don't generally,. We are adding it to the template here because we want to allow sites/themes/etc. to override all this. We could construct some of the various strings in the preprocess as well if needed; I just liked how nice and readable it was with the template and that themers could customize their own template and text entirely. This is very much not my area of expertise though so it could be better to assemble the longer strings in the preprocess and pass them in. I might look at that next.
Me neither but Ted added that and it made it work so... :D
I prefer them for readability when there are comparisons of long things, though not super fanatical about it. :) I've left it for now.
Deliberately did not do this. :) Translating plurals within another string is fraught because in many language the plural depends on where it is in the sentence etc. So we should avoid it where it's not strictly necessary.
@davidhernandez said we haven't done this historically so I have left that out. :)
Attach refactors the tests more and also addresses #53 and #55.1 and 6. #54 is not yet addressed, nor the general question about what strings to put where.
The interdiff for the test is a bit messy because I moved some lines around, so probably easier to review the test as a whole.
Comment #57
phenaproximaThat test is great. I really like it. Super thorough, and clear too!
Should probably use assertSame() when checking strings; there's no real reason not to. We should also assert the legend exists (rather than courting fatal errors by assuming it does), like this:
$this->assertSame($field->getLabel(), $this->session->elementExists('css', 'legend', $fieldset)->getText());
Doc comment seems to be out of date :)
What if $text contains apostrophes/single quotes? Should we escape it first?
Not a big deal, but it seems weird for this to be a wrapper around assertHelpLink(). It could just be as simple as:
$this->session->elementNotExists('named', ['link', $text], $element)
Comment #58
xjmAttached addresses ##57. #54 is not yet addressed and the template could also use some accurate documentation of the available variables.
Comment #59
tedbowHere is patch that just shows the media changes for review as the field API changes are being worked on in #2940201: hook_field_widget_form_alter() can no longer affect the whole widget for multi-value fields
Look through @phenaproxima review in #57
All fixed.
For 4)
\Drupal\Tests\media\Functional\MediaUiFunctionalTest::assertNoHelpLink
no has another check so I think it should not be removed now.Comment #60
tedbowand
Oh no!
See #41 for where I got this idea from. I think some who knows the theme system better me to confirm that this is the proper way to do this.
We should add a
break
here after$create_access = TRUE;
.We just need to know if the user has access to create 1 bundle. Check this access could be expensive if there are implementations of
hook_entity_create_access
If the user has access to add only 1 bundle then we should probably make the link to the add page just for that bundle to avoid confusion.
The user could access only 1 bundle for this field but then other bundles that can't be referenced here.
Would it be better to check access against the actual route like
Url::fromRoute('entity.media.collection')->access()
Then if access on the route is altered this would be picked up. Not positive on this but it seems more robust.
Comment #61
tedbowAddressing #60
$create_access
but rather$create_bundles
because we need to keep track of whether the user has access to create 0, 1 or more than 1 bundles.So breaking on
count($create_bundles) > 1
entity.media.add_form
if the user has access to create just 1 bundleComment #64
tedbowIn #61 I forgot to update the tests. So this patch does:
\Drupal\Tests\media\Functional\MediaUiFunctionalTest::providerTestMediaReferenceWidget
to use keyed indexes because this is easier for debugging and test error results\Drupal\Tests\media\Functional\MediaUiFunctionalTest::testMediaReferenceWidget
Comment #65
xjmLatest proposal in #2940201: hook_field_widget_form_alter() can no longer affect the whole widget for multi-value fields is to not include the cardinality in the context and just reference it from the form element array. So just making sure that works here.
Comment #67
tedbowJust a typo in variable name in #65
Comment #68
xjmBTW per @davidhernandez's suggestions, we could move these two strings into variables and that would be a more normal amount of text in the template. (I do think the headers belong in the template for sure.)
Comment #69
xjmGoing to work more on that and on the other template improvements @davidhernandez suggested. Also just realized I accidentally left the issue assigned to myself, but re-assigning now. :)
Comment #70
samuel.mortensonThis is pretty good, and is an improvement on the standard entity reference field. Nice job 👍
1. One thing I noticed is that the name of the field is used for the "Create new ..." and "Use existing ..." text, which looks OK when the field is named something like "Media", but when you use something more specific (see below), it can look overly repetitive (and may be grammatically incorrect, i.e. "Create new "Vacation pictures" media").
Instead of using the field name, could we use the label of the bundle if only one bundle is targeted, or just "media" if multiple bundles are targeted?
2. The "Create new ..." text uses an h4 tag, which makes it larger than the "Use existing ..." label. Is this intentional? Could we just a strong tag instead?
3. The "Allowed media types ..." text uses the machine name of the Media types, instead of the label. Would look a bit nicer to use the label.
Example for this comment:
Comment #71
xjmNot intentional. TBH I did not even notice. :) I think the
h4
is more semantic (and also what we use elsewhere within fieldsets) but we can probably style the tag to match the legend.Comment #72
xjmOh ah, regarding:
This is a pretty great idea actually. The fieldset legend already provides the title, and we already call it "media" elsewhere including elsewhere in the patch. I think we could probably just use "Create new media" and "Use existing media" consistently for simplicity.
Comment #73
xjmAttached addresses #54 and #70.
Comment #74
xjmComment #75
xjmIndentation change here was not intentional. But not uploading a new patch just yet in case this gets more feedback. (Whitespace doesn't need to block RTBC in any case.)
Comment #76
xjmOnce #2940201: hook_field_widget_form_alter() can no longer affect the whole widget for multi-value fields is committed (RTBC now), #75 can be also fixed in that reroll.
So close!
Here are some updated screenshots:
Single element
Multiple element limited to 3 values
Unlimited multivalue
Comment #77
xjmAlso updating the IS.
Comment #78
xjmSaving issue credit for the many reviewers.
Comment #79
xjmAlright, the blocker is in now (with a slight API change), so I'm updating this one.
Comment #80
xjmHere it is!
Comment #81
davidhernandezThese ifs shouldn't be necessary. There is no markup being printed inside them, only Twig variables.
It is needed, for example, for the button and h4.
Also, no text in the template. yay \0/
Comment #82
benjifisherJust looking at the interdiff,
The docblock should be updated to match the new hook name.
Sorry, I do not have time to do a real review.
Comment #83
benjifisherThis makes the changes requested in #81 and #82. I have no opinion on #81.
Comment #84
xjmThanks @benjifisher and @davidhernandez.
Comment #85
samuel.mortensonAll my previous feedback has been addressed, and the patch is looking good now. Marking as RTBC, pending further review.
Comment #86
yoroy CreditAttribution: yoroy at Roy Scholten commentedContent wise this looks ok. For me "then add it by name *to* the form" felt slightly off, was thinking "then add it by name *in* the form below. I'll defer to the native speakers, no biggie I think.
Is there a reason why the texts below the form elements ("Type part of…") are not shown as actual descriptions for those fields? Currently these texts have a larger gap between them and the form field above, again looking slightly off.
Comment #87
xjmThanks @yoroy.
"To" is definitely more idiomatic than "in". I would never use "in" in this context. :) (Prepositions are indeed the least logical or predictable part of any language I've studied...)
This is already inside the actual description div, similar to any field's. I think the gap is just a side effect of a
<p>
tag being wrapped around the text (which I added belatedly after remembering that the HTML spec requires (or required, back in the day) all text be wrapped in<p>
and that<p>
as a direct child of a<div>
was not technically correct.However, if we don't do that elsewhere in core, we shouldn't do it here. Looking into this now; if we don't use the
<p>
for other descriptions we shouldn't here; and if we do then maybe I'm missing a class that is supposed to be on it or its parent.Comment #88
xjmLooks like we don't use the
<p>
elsewhere by default (although I believe the site builder can add such markup to descriptions if they want), so I'm going to remove it.Comment #89
xjmAha. So a very similar case in core is link fields, which are another similar reference field. They have one description provided by core that must be shownalongside any custom description the site builder might add.
If there is no custom help text, it's just displayed in a div, no parent.
If there is a custom text as well, it is displayed in an unordered list.
So I'll update our handy template to do just that.
Comment #90
xjmOkay that took longer than I thought. Turns out there was a bug in the preprocess that was preventing the description class from being added. Attached fixes that, removes the
<p>
that are not used elsewhere in core and also adds the<ul>
in the same pattern as it's used for link fields.Comment #91
xjmComment #92
xjmOops, forgot to embed:
Comment #93
davidhernandezWhere did this wild BR come from? It shouldn't be needed for spacing following an H4. Is there some CSS forcing that inline? I didn't notice any in Seven with a quick look.
Comment #94
tim.plunkettI don't understand the need for these 3.
How do they improve upon the attribute handling in template_preprocess that will run for this first?
Could use some comments.
What about using the EntityTypeBundleInfo service? Or retrieving the labels directly from the element? See \Drupal\Core\Entity\Plugin\EntityReferenceSelection\DefaultSelection::buildConfigurationForm().
NWing for this
Why does this need to be stored? If it's a helper, I've never seen it done this way, I'd recommend not doing it and having a
at the top of each method.
If this is needed for some actual functional reason, that is scary and should be explicitly documented.
Nit: start with "Tests"
Nit: This is deprecated.
Comment #95
tim.plunkettComment #96
xjmRe: #93:
Yeah that is coming from the standard
.label
class that was added for #70.2 (AFAIK the standard class for formatting not-label things like labels.) I'm definitely trying to not add CSS that is nonstandard and to reuse the existing markup patterns... but if you know of a better one please let me know. :)Re: #94
Those are copied from
core/includes/form.inc
to address #54 because we are adding our own fieldset while overriding the field form template. Also not commented there, but at least we can add a comment to that effect. :)Updating the patch for these things.
Comment #97
xjm(Adding other reviewer credit.)
Comment #98
xjmAttached addresses #94. I did not remove the feral
<br />
as yet... (edit) as I am not sure what to do about that TBH.Comment #99
xjmJust fixing a pretty steep case of run-on sentence.
Comment #100
xjmAlso fixing embedding failure in the IS.
Comment #101
xjmI realized this morning that we need to use the generic hook instead of the specific one, because the same text should be shown for (e.g.) the autocomplete tags widget.
Making that change now.
Comment #102
xjmOkay wow. @tim.plunkett and I spent a bunch of time debugging this.
The first problem was that we were applying the help text to the default widget (
entity_reference_autocomplete
), but ignoring the other three widgets available to entity reference fields in core. To address this, we move up fromhook_field_widget_mutlivalue_WIDGET_TYPE_form_alter()
to the generichook_field_multivalue_form_alter()
and check the field type (entity reference), target referenceable entity type (media), and widgets (entity_reference_autocomplete
,entity_reference_autocomplete_tags,
etc.)The second problem was that the hook was being invoked in the wrong place within
WidgetBase
. There are actually three scenarios possible for widgets:To resolve this, we need to move the hook invocation. This will also need its own issue and tests.
The third problem was that the render array structure is utterly unreliable from widget to widget. The logic added in earlier patches unfortunately did not hold up under further usecases. With @tim.plunkett's help I changed all this to instead use the reliable objects that are available within the alter hook.
To avoid passing lots of information to the preprocess in magic keys, I instead moved the logic back into the alter hook, and the preprocess now retrieves the relevant template variables passed to it in an array stored in
$elements['#media_help']
. This means the preprocess also becomes a lot cleaner, and we only have to debug and adjust business logic in one place rather than two. (@tim.plunkett had proposed removing the#
from the child keys of$elements['#media_help']
, but when I tried that they were stripped between the alter and preprocess, so I've left them for now.The fourth problem is that
template_preprocess_field_multivalue_form()
assumes the presence of an$element[#cardinality_multiple]
key that does not actually exist for multivalue widgets. For now I'm just initializing it in the alter hook, but this seems like it needs a followup...The fifth problem is that element array structure of the options widgets does not seem to work with the multivalue field hook/template. However, select lists of options are less baffling to the user, so I think addressing this can be scoped to a followup.
This needs more tests, too. I think we will want to add the widget as another parameter for the data provider.
Comment #104
xjmSecond "patch" is actually just the interdiff, sorry. But this is NW for tests and scoping anyway.
Comment #105
tim.plunkettYay early returns!
Unless I'm missing something, there is an unwritten else case here in which $add_url will be an undefined variable. Aka, when there are no bundles?
Adding $add_url = NULL; above and wrapping the last line in
if ($add_url)
would helpThe double nested #-prefixed keys look off to me. What's the reason this is needed at the interior level?
Whenever I see code digging this deep I think of a skier on a slalom course. Not sure why. But, yikes! (also nothing to be done about that here, this is correct AFAIK).
If the # prefixes from above are removed, this substr call can be dropped. And in fact, the whole thing could be
$variables += $element['#media_help'];
The need for this change is not clear to me, but seems okay?
Comment #106
xjm#105.3 and 5 are already answered in #102.3. :)
For #105.6, I just decided the existing variable name was not specific/clear enough.
I'll fix #105.2 once I finish writing tests and @tedbow files our friendly followups.
Comment #107
tedbowCreated follow up issues and update @todo in patch
#2943020: Inform content authors where they can list and add media on all media reference widgets
#2943024: Ensure widget title is correct when adding media reference help text and links
UPDATE: sorry was not on 8.6.x when I created my new issue branch. will upload a new patch
Comment #108
tedbowHere is the correct patch for #107
Comment #109
tedbow#102.2 created issue #2943035: hook_field_widget_multivalue_form_alter and hook_field_widget_multivalue_WIDGET_TYPE_alter are not invoked for single widget that handle multiple values
Comment #111
tim.plunkettUnused
I think this function deserves a nice
at the top, so all further calls will typehint correctly.
Super unimportant nit, but
$add_url = Url::fromRoute('entity.media.add_form', ['media_type' => $create_bundles[0]])->toString();
is way more commonThis is unused.
Too many t's
This is what's causing the failures.
In WidgetBase, if the cardinality is > 1, the " (value $delta)" part seen in the failures is appended to the title.
Additionally, it is explicitly marked as
'#title_display' => 'invisible'
with a comment about how it's not expected to be rendered.If you want the correct title,
$field_definition->getLabel()
has it.First one is "autcomplete". Also, out of context that sentence reads a bit snarky :) (but there's totally more text below it and it's fine)
Comment #113
xjmRe: #111.2 those are a PHPStorm thing I guess. Not quite sure what to add as the datatypes don't match (
getFieldDefinition()
in the proposed snippet vs.getType()
in the code).Attached addresses the rest of #111 plus adds test cases for the tags-style widget. (The fix for #111.6 is from @tim.plunkett.)
Comment #114
xjmAlright, I finally added the tests we've needed from #26 / #29. This coverage is pretty important because we've repeatedly broken the field settings form over the course of this issue, including in the most recent patches. To ensure it doesn't regress again, we create the media reference field through the UI rather than the API.
The fix has two parts. First, we should not be altering the field default values on the field settings form. That form is for site builders rather than content authors, and a lot of the info we would add is just not relevant, so it mostly adds noise and overhead.
hook_field_widget_multivalue_form_alter()
and friends already include a mechanism to distinguish the settings default form from the actual form with$context['default']
. (It was added because of a bug very much like this one that was fixed in late 2011).Secondly, I had a fragile line code that was trying to reference an array key that could be an undefined index. That's just bad PHP code, so I've fixed that as well to avoid other issues down the road.
I think this is finally ready (once the blocker lands and we reroll this on top of it).
Comment #116
xjmSame patch, rolled on top of the fix to
WidgetBase
.Comment #117
samuel.mortensonLooks good to me 👍
Comment #118
larowlanScreenshots look like a good interim step.
Was there a reason why we didn't add new purpose built widgets that extend these instead of going down the alter path?
We could limit their availability to just media ER fields with the isApplicable method.
If the answer is, because we intend to remove all of this in the future and doing it as an alter hook means we don't have to support it forever but doing it as a stand-alone widget would mean we're supporting it till Drupal 9 - +100.
The theme system does this for you, if you declare the variable names in 'variables' in your hook theme.
Was there a reason we went with 'render element' instead of 'variables'?
Each of these provider cases installs a new Drupal site. Are we ok with that?
If not, we can move to a doTestSomething() style approach where we feed the cases in, but have only one Drupal install for performance sake.
Given we don't really need isolation here, because we aren't creating any content - we can change the config between each case.
Comment #119
xjmThanks @larowlan.
The reason is that we want to be able to provide the text on existing widgets, rather than creating a new one that the site builder would need to know to select. For example, we'll want the help text on both regular autocomplete and tags-style autocomplete widgets, and we can eventually add the help text about creating to options widgets as well. That's why we use an alter hook.
Doing it this way also allows us to avoid API surface (as you've said), minimizes the disruptions/additions since this is an interim fix to replace all this with the Media Library Browser, and still allows module developers, themers, and site owners to customize the help text in whatever way they like (which is something else that came up in usability review).
Hm, I don't understand the question. The line you list is only adding a small subset of the elements array, passed over from the alter hook where they were calculated. There are however other variables for the correct fieldset and form markup attributes that we're setting above. (We're overriding the field multiple form template, but incorporating markup and templating from the fieldset template.) Also there is both a render element and variables?
I did it this way on purpose because we had previous regressions on the issue where tests did not fail because there was a false positive from something else on the page. I've been running the test locally with its provider since I added it and it really doesn't take long at all. And developer time is after all more costly than hardware. :)
Comment #120
effulgentsia CreditAttribution: effulgentsia at Acquia commentedIn that case, how about adding
@internal
tomedia-reference-help.html.twig
with a link to the issue that will remove it? I also wonder if the theme system would allow us to prepend an_
to the theme hook name (-
to the .html.twig name) as a further hint to contrib that this will get removed.Comment #123
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI'm ok with #120 being explored/done in a post-beta followup. Therefore, pushed #116 (plus the coding standards fixes in this interdiff) to 8.6.x and 8.5.x.
Comment #124
xjmThanks @effulgentsia! Regarding #120, I still would not remove the template in a minor release. We should avoid using
@internal
in that way; the maintenance burden is not high enough to justify the disruption. And actually I don't want to mark it internal because we do want to let people override it. The media library will add a different widget that will become the default for new installs, but this help text will still be useful for the autocomplete widgets. So I did not file said issue. :)