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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

phenaproxima created an issue. See original summary.

phenaproxima’s picture

Status: Active » Needs review
FileSize
2.74 KB
samuel.mortenson’s picture

We should use the plural label for the entity here

Edit: Read the patch wrong, when I saw @entity_type I assumed that was the Entity Type label, not the plural label. Carry on.

phenaproxima’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Okay. I'll just write some tests, and we'll be good to go.

samuel.mortenson’s picture

What happens when a field has unlimited cardinality? Should we have a case and label for that as well?

phenaproxima’s picture

In 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.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
7.21 KB
4.34 KB

Okay, added some tests. Let's see how this goes.

Status: Needs review » Needs work

The last submitted patch, 7: 2877751-7.patch, failed testing.

phenaproxima’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
7.19 KB
716 bytes

Okay, 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 :)

samuel.mortenson’s picture

I 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 list
2. Added the same message to the FileBrowser widget, to support files/images

phenaproxima’s picture

Looks good to me!

thelmer’s picture

This 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..

phenaproxima’s picture

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..

Can 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".

thelmer’s picture

Our patch to fix things temporarily:

-        '@entity_type' => $target_type->getPluralLabel(),
+        '@entity_type' => 'entities',

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:

return (string) $message;
phenaproxima’s picture

The 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?

ZeiP’s picture

The patch worked well and is otherwise OK. I'd change the status to RTBC, but perhaps we need to wait for @thelmer's response.

phenaproxima’s picture

FileSize
7.75 KB

Re-roll against the 8.x-1.1 tag, which mysteriously diverges from HEAD. Go figure.

Status: Needs review » Needs work

The last submitted patch, 17: 2877751-17.patch, failed testing. View results

adinac’s picture

On 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."}

thelmer’s picture

We 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

phenaproxima’s picture

Issue tags: +Needs reroll
phenaproxima’s picture

@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.

thelmer’s picture

Hi phenaproxima
I was actually just found this issue you refer to, it fixes my problem.

s_leu’s picture

Needs a re-roll for 2.x branch

Ivan Berezhnov’s picture

Issue tags: +CSKyiv18
Martijn de Wit’s picture

FileSize
7.75 KB

Made 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.

Martijn de Wit’s picture

FileSize
7.78 KB

New 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

Martijn de Wit’s picture

Version: 8.x-1.x-dev » 8.x-2.x-dev

Set issue version to 8.x-2.x-dev. Because that version is targeting the newest Drupal core versions.

Martijn de Wit’s picture

Status: Needs work » Needs review
balsama’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
6.35 KB

Hi. 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.

Martijn de Wit’s picture

Issue tags: -Needs reroll
saltednut’s picture

+1 for the RTBC, can we get this committed?

phenaproxima’s picture

Issue tags: +Nashville2018

I'll try to get this landed at the Nashville code sprint today.

samuel.mortenson’s picture

Status: Reviewed & tested by the community » Needs work

The 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?

Martijn de Wit’s picture

8.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.

samuel.mortenson’s picture

@martijn-de-wit 8.x-1.x is for users who haven't migrated to core Media yet, from what I understand.

Martijn de Wit’s picture

Ah 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

maximpodorov’s picture

The patch in #27 deletes empty lines in such way that other patches for Entity Browser can't be applied cleanly.

maximpodorov’s picture

Status: Needs work » Needs review
FileSize
5.8 KB
2.24 KB

So the new patch is the patch from #27 without empty line deletions.

Rajab Natshah’s picture

FileSize
5.21 KB

Updated the patch to work with Entity Browser 8.2.2

Status: Needs review » Needs work

The last submitted patch, 40: 2877751-40.patch, failed testing. View results

saltednut’s picture

We 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.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
5.17 KB
2.12 KB

Let's see if this brings the test back from the red.

rlnorthcutt’s picture

Status: Needs review » Reviewed & tested by the community

Tests look good!

samuel.mortenson’s picture

+++ b/src/Plugin/Field/FieldWidget/EntityReferenceBrowserWidget.php
@@ -675,6 +676,42 @@ class EntityReferenceBrowserWidget extends WidgetBase implements ContainerFactor
+      $message = t('You can select up to @maximum @entity_type (@remaining left).', [

Should use $this->t(). I'll fix on commit.

  • phenaproxima authored 8249edb on 8.x-2.x
    Issue #2877751 by phenaproxima, Martijn de Wit, samuel.mortenson,...
samuel.mortenson’s picture

Status: Reviewed & tested by the community » Fixed

Thanks all.

oknate’s picture

Testing against 8.x-1.x branch for backport.

  • oknate committed 892d0ce on 8.x-1.x authored by phenaproxima
    Issue #2877751 by phenaproxima, Martijn de Wit, samuel.mortenson,...
oknate’s picture

The test failures are due to #3080163: Decouple TestFileCreationTrait::getTestFiles() from simpletest module

\Drupal::service('file_system')->copy(\Drupal::root() . '/core/modules/simpletest/files/image-test.jpg', 'public://example.jpg');

This file got moved in 8.8. I'll create a separate issue for that.

#3086078: Failures due to moved file in Drupal Core

samuel.mortenson’s picture

Thanks @oknate!

Status: Fixed » Closed (fixed)

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