Create new version compatible with core Media module

Here a first version of the port.

In manual testing, I'm having some issues to upload files to the API, that disappear when I step through the code with the debugger, so I'm assuming some race condition must be happening. I have the same error with the current 8.x-1.x HEAD, though, so I wonder if it's due to my local setup or the testing account I'm using.

Some review, testing and feedback with this patch would be appreciated.

Simplifying the check a bit.

1. +++ b/bynder.info.yml
   @@ -6,7 +6,7 @@ core: 8.x
      - dropzonejs:dropzonejs
   

   should we also specifiy that this needs 8.x-2.x here?

2. +++ b/bynder.install
   @@ -42,8 +42,20 @@ function bynder_requirements($phase) {
   +    // When reinstalling the media module we don't want to copy the icons when
   

   copy paste leftover (we shouldn't have deleted that function ;))

@Berdir thanks for reviewing!

We can make that a requirement to reduce confusion, but technically we don't depend on the new EB widget that was added in 2.x, so EB 1.x should work fine as well.

But it's true that if a site is using Media in core, it's probably a good idea to have all modules that deal with media entities in their 2.x branches.

Apparently, we weren't requiring Entity Browser in the info file, so I'm adding the entry with the 2.x restriction.

Thanks!

I think the dependency to entity_browser is kind of optional and only the demo module has an explicit dependency on it?

Oh that could indeed be the reason it was not required before.

However, unless I'm missing something, being the main integration with the API done through EB widgets (search / upload), I wonder what the use cases for using this module without it are? We would have a Media type (bundle) and a formatter, but all entities would need to be created through the standalone form, entering the remote UUID by hand?

Also, in the project page we state that Entity Browser is a requirement, so users probably already expect that. I believe it may make sense to include it as a hard requirement as well in the main module.

Thoughts?

Just remembered something relevant to take into account if we keep those minimum version restrictions in the info file.

Drupal doesn't parse info file versions for projects not packaged on Drupal.org, so if a site is using DropzoneJS or Entity Browser checked out from git or a -dev package, those restrictions will not be met, even if they are on the 2.x branches. Not sure if this is an issue in this case, but thought it would be worth mentioning here.

This prevents the race condition issue I experienced in some scenarios with the upload widget.

Hi!

Yes, I belive it is a good idea to release this as a 8.x-2.x branch, to make a clear difference from the existing branch. Other modules that integrate with Media Entity in contrib are doing the same for their branches compatible with media in core.

diff --git a/modules/demo/config/install/core.entity_view_mode.media.thumbnail.yml b/modules/demo/config/install/core.entity_view_mode.media.thumbnail.yml
deleted file mode 100644

Maybe instead of removing the file, we could move it into the modules/demo/config/optional/ instead? This way sites not using the lightning profile would get it, and it wouldn't be installed if already present.

Here's a patch that implements the suggestion in #14

Thanks for the feedback!

Yep, the order is important here... moving all of them to optional seems to be enough to have all of them enabled at the same time, and they won't try to override an existing config object that may exist with the same name.

Hi João,

I'm trying to reproduce the issue, but I'm not being able to connect to the API. Is there any connectivity issue at this moment?

Server error: `GET https://*****.getbynder.com/api/v4/media/?limit=15&page=1&type=image&total=1` resulted in a `500 Internal Server Error` response:
{"message":"An error occurred","statuscode":500}

Thanks!

Could you please share steps to reproduce?

I have just tested again in a new install (standard profile), and although I still have the API errors like:

Method: getMediaList throws error with message: Server error: `GET https://*****.getbynder.com/api/v4/media/?limit=15&page=1&type=image&total=1` resulted in a `500 Internal Server Error` response: {"message":"An error occurred","statuscode":500}

Method: getMediaInfo throws error with message: Server error: `GET https://*****.getbynder.com/api/v4/media/C634B855-B97A-405D-AF9839DF4457621C/` resulted in a `500 Internal Server Error` response: {"message":"An error occurred","statuscode":500}

which prevent the upload from finishing, I don't experience the errors you mentioned in #18. Any additional info to help reproducing it would be appreciated.

Thanks!

Hi João, I'm still getting the error 500 today. Could you please double-check?

Here is what I'm doing: video

Could it be something related to the test account we are using? Could you please try with the same test account so we can see the differences?

Thanks!

(Note: on the video, when I type "dcdr", this is an alias for "drush" in my environment)

Thanks for solving the issue with the API account!

Now I can indeed reproduce the issue, and apparently it's due to the same racing condition we were experiencing above. In this issue we have introduced a workaround in #11 to avoid it in some scenarios, but it appears that now when it happens (trying to save the entity twice, with a duplicate UUID error message), we avoid that error but both transactions are rolled back somehow and the Entity Browser widget ends up not saving the entity, passing NULL to the field widget, which ultimately produces the error you described above.

However, as I mentioned before, I can reproduce the same racing conditions while testing with the current 8.x-1.x (without the patch): video

Could you please test and see if it also happens for you in the current dev version of the module? (i.e. using Media Entity contrib)

Thanks!

So this is the patch that should fix the race conditions. It should apply to both 1.x (current code) and 2.x (after patch #17).

I can. Committed the conversion patch to the new 8.x-2.x branch/release, as well as comitting the race condition fix to both branches.

https://www.drupal.org/project/bynder/releases/8.x-2.x-dev

A side effect of moving all the config to /optional, is that we have an install profile that depends on the bynder module and adds fields to the media type, but we get an order issue:

When you’re doing a full site installation (i.e., install.php or drush site-install), optional config is treated a little bit differently. In such a case, all extensions are installed as normal, but their optional config is ignored initially. Then, at the end of the installation process1, once all extensions are installed (and their default config has been imported), all2 the optional config is imported in a single batch.

https://lightning.acquia.com/blog/optional-config-weirdness-drupal-8

I've moved any config that requires the Bynder media entity to our own /optional directories, which seems to resolve it, but it doesn't seem ideal. Maybe we should be copying and installing that confirm from our install profile itself instead of relying on the Bynder module's configuration?