Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
The Entity Browser field widget supports multi-value fields, but it doesn't tell the user how many items the field can take, or how many more items they can select after they have selected a few. This would be really useful information to give the users, and it'd be quite simple to do. Niceties are good :)
Patch forthcoming.
Comment | File | Size | Author |
---|---|---|---|
#43 | interdiff-2877751-40-43.txt | 2.12 KB | phenaproxima |
#43 | 2877751-43.patch | 5.17 KB | phenaproxima |
#40 | 2877751-40.patch | 5.21 KB | Rajab Natshah |
#39 | 2877751-interdiff-27-39.patch.txt | 2.24 KB | maximpodorov |
#39 | 2877751-39-8.x-2.x.patch | 5.8 KB | maximpodorov |
Comments
Comment #2
phenaproximaComment #3
samuel.mortensonWe should use the plural label for the entity hereEdit: Read the patch wrong, when I saw
@entity_type
I assumed that was the Entity Type label, not the plural label. Carry on.Comment #4
phenaproximaOkay. I'll just write some tests, and we'll be good to go.
Comment #5
samuel.mortensonWhat happens when a field has unlimited cardinality? Should we have a case and label for that as well?
Comment #6
phenaproximaIn my opinion, no. If you have unlimited cardinality, there's nothing that needs to be said; you can select as many things as you want, and that's that.
Comment #7
phenaproximaOkay, added some tests. Let's see how this goes.
Comment #9
phenaproximaOkay, figured out why the tests were failing: it's because I wrapped the message in a P tag and the other tests are brittle about that kind of thing. We gots to rearrange that...but not in this issue :)
Comment #10
samuel.mortensonI made two tweaks:
1. Wrap the cardinality message in a
<p>
tag, avoiding test failures and jQuery UI draggable issues by prefixing it to the entity list2. Added the same message to the FileBrowser widget, to support files/images
Comment #11
phenaproximaLooks good to me!
Comment #12
thelmer CreditAttribution: thelmer at Adapt commentedThis patch breaks all ajax request on our pages.
We get a: Uncaught PHP Exception LogicException: "The database connection is not serializable.
It is somehow related to '@entity_type' => $target_type->getPluralLabel()' in EntityReferenceBrowserWidget.php
The object returned by getPluralLabel can't be serialized by the string typecast performed when getCardinalityMessage returns the result.
If we replace getPluralLabel() with a string everything works..
Comment #13
phenaproximaCan you provide an example of the replacement code you created? I believe that getPluralLabel() returns some sort of MarkupInterface object (none of which, as far as I know, have anything to do with the database), so it should be pretty harmless to cast it to a string and use it that way. I'm not sure what you mean by "serialized by the string typecast".
Comment #14
thelmer CreditAttribution: thelmer at Adapt commentedOur patch to fix things temporarily:
Somehow deep down in the object returned by the t function called by getPluralLabel somthing can't be casted to a string.
In this case the object returned contains a database connection and when trying to make this into a string it's being serialized.
The database connection object has an explicit safeguard against being serialized and it's the exception thrown by this guard we see.
The cast happens when getCardinalityMessage returns:
Comment #15
phenaproximaThe part I don't get here is this: if the object returned by getPluralLabel() had a database connection anywhere, it seems quite likely that we would have caught this bug during both automated and manual testing. So I strongly suspect it is something about your system setup that is breaking this. Could it be another module that you have installed? Perhaps some custom code? Can you provide some more detail about how your site is set up? Specifically, which modules you have turned on, and which theme you're using to display the entity browser?
Comment #16
ZeiP CreditAttribution: ZeiP at Citrus Solutions Oy commentedThe patch worked well and is otherwise OK. I'd change the status to RTBC, but perhaps we need to wait for @thelmer's response.
Comment #17
phenaproximaRe-roll against the 8.x-1.1 tag, which mysteriously diverges from HEAD. Go figure.
Comment #19
adinac CreditAttribution: adinac as a volunteer commentedOn a paragraph bundle, I added a media reference field which uses the entity browser widget. The patch from comment #17 causes an error when trying to add a media entity via the entity browser:
AjaxError:
An AJAX HTTP error occurred.
HTTP Result Code: 500
Debugging information follows.
Path: /node/1/edit?ajax_form=1
StatusText: Internal Server Error
ResponseText: {"message":"A fatal error occurred: The database connection is not serializable. This probably means you are serializing an object that has an indirect reference to the database connection. Adjust your code so that is not necessary. Alternatively, look at DependencySerializationTrait as a temporary solution."}
Comment #20
thelmer CreditAttribution: thelmer at Adapt commentedWe have just seen the error with serializable database connection again after upgrading a site.
It's the same issue as i reported in #12 and is reported by adinac in #19
Comment #21
phenaproximaComment #22
phenaproxima@thelmer, that is due to a bug in core. #2893029: EntityType objects cannot be reliably serialized without DependencySerializationTrait is the dedicated issue for that bug, and there is a patch. Try applying that and seeing if the issue persists.
Comment #23
thelmer CreditAttribution: thelmer at Adapt commentedHi phenaproxima
I was actually just found this issue you refer to, it fixes my problem.
Comment #24
s_leu CreditAttribution: s_leu at Station commentedNeeds a re-roll for 2.x branch
Comment #25
Ivan Berezhnov CreditAttribution: Ivan Berezhnov as a volunteer and at Drupal Ukraine Community for Levi9 commentedComment #26
Martijn de WitMade a new patch for 8.x.2.x. Used #17 as starting point, as it did not applied to the 8.x.2.x dev or alpha 2 release.
Comment #27
Martijn de WitNew patch to apply with:
Commit: 1bda49d1f79b33c0b8dd0fc3fd8faf7a02a2a0b7 [1bda49d]
Parents :c7fa4e992f
Author: marcoscano
Date: 1 March 2018 at 14:56:09 CET
Committer: Primsi
Labels: HEAD 8.x-2.x origin/8.x-2.x
Comment #28
Martijn de WitSet issue version to 8.x-2.x-dev. Because that version is targeting the newest Drupal core versions.
Comment #29
Martijn de WitComment #30
balsamaHi. We've been using the patch in #17 with the 2.0.0-alpha2 version of this module in Lightning for about 6 months along with some functional test coverage. So RTBC from me.
Here's an interdiff for 17-27 for clarity.
Comment #31
Martijn de WitComment #32
saltednut+1 for the RTBC, can we get this committed?
Comment #33
phenaproximaI'll try to get this landed at the Nashville code sprint today.
Comment #34
samuel.mortensonThe patch from #27 applies cleanly to 8.x-1.x but does not pass tests - could someone look at the fails or roll a newer 8.x-1.x patch if possible?
Comment #35
Martijn de Wit8.x-1.x is targeting Drupal Core > 8.4.X right? I can't select Drupal 8.3.X any more in the automated tests. (probably because it's a to old version.) Ran a test for D8.4 but that is also failing, though other error's than D8.6.x.
Comment #36
samuel.mortenson@martijn-de-wit 8.x-1.x is for users who haven't migrated to core Media yet, from what I understand.
Comment #37
Martijn de WitAh ok, so is it possible to use Entity browser 8.x-1.x with D8.5.X and Media entity.
If the maintainers still want to support 8.x-1.x and add new features to it, than we have to fix the tests for the patch. Someone else has to jump in to fix those test error's, my knowledge lies mainly with front-end development. thank you for the quick respsonse
Comment #38
maximpodorov CreditAttribution: maximpodorov commentedThe patch in #27 deletes empty lines in such way that other patches for Entity Browser can't be applied cleanly.
Comment #39
maximpodorov CreditAttribution: maximpodorov commentedSo the new patch is the patch from #27 without empty line deletions.
Comment #40
Rajab Natshah CreditAttribution: Rajab Natshah at Vardot for Vardot commentedUpdated the patch to work with Entity Browser 8.2.2
Comment #42
saltednutWe are constantly dealing with this patch in builds and have had to do some insane patch combining just to get things working due to the overlap with #2825555 resulting in having to build currently using this combined patch: https://www.drupal.org/files/issues/2018-12-27/2877751-39_2825555-20_com...
If there is anything we can do to help get this in, it would clear up a ton of DX problems that my team (and probably others) are running into.
In general, why stuff like this languishes when its known how much an issue media patches and shuffling can be, I don't know. It's like we love creating extra work for ourselves? Anyway, let me know what I can do to get this patch forward, I won't RTBC it yet but I will say I've been using it in various forms for like two years now.
Comment #43
phenaproximaLet's see if this brings the test back from the red.
Comment #44
rlnorthcuttTests look good!
Comment #45
samuel.mortensonShould use $this->t(). I'll fix on commit.
Comment #47
samuel.mortensonThanks all.
Comment #48
oknateTesting against 8.x-1.x branch for backport.
Comment #50
oknateThe test failures are due to #3080163: Decouple TestFileCreationTrait::getTestFiles() from simpletest module
This file got moved in 8.8. I'll create a separate issue for that.
#3086078: Failures due to moved file in Drupal Core
Comment #51
samuel.mortensonThanks @oknate!