🇧🇪Belgium @keszthelyi

Hi @nagy.balint,

This is responsible for the caching of the library assets, so normally it should always be the same version as the chosen library version. If it's removed that means there won't be browser cache problems, but it might force the user's browser to download chosen.js when it's not needed (because then it will change the version hash every time the module is updated, and module updates don't always update the chosen library version).

So ideally it should be kept, but it's important to update it every time the chosen library version is updated.

keszthelyi → created an issue.

Patch from MR.

On a second thought, the version number in this case should not be following the chosen library version, but the module version. As this is defining the drupal.chosen assets version and not the library version. The caching issue itself is related to the module's js file (and not the chosen library js). The problem is that the browser returns the cached 4.x version of the module's js.

However, I think the ideal would be to remove the version property (not defining any version). By doing this, Drupal will use an automatically generated query string added to filenames that changes on every update or full cache flush. Like this it won't be needed to always update the version number each time the assets are changed in a new module version. See this issue for entity browser 🐛 Remove VERSION from libraries.yml Fixed for more details.

Tested the MR on our project (on latest commit) and it works, the title is correctly added to the iframe.

I tested the MR on our project where the issue is reproducible, and after applying the changes the caching issue is fixed.

Re #11: it seems that in this scenario the content is always locked. After breaking user B's lock, user A is redirected to content edit form, locking the content again (by user A). When user B visits the break lock form the content is locked by user A and user B doesn't have permission to unlock the content, so the access denied is technically correct. User B can only avoid the access denied (with this patch) if in the meantime user A also unlocks the content after breaking user B's lock.

The access denied in the scenario described in #11 could only be avoided if access is always allowed to the form and everything is handled in buildForm() with redirects, but not sure if that's the better solution.

Attaching a reroll of #15 for 3.0.0-alpha2.

Fixes an issue with the original #15 patch that causes error when the redirect Url generated from the destination parameter is not a routed Url (so the getRouteName() method can't be called on it). This can happen for example if the base path of the site contains a subdirectory, but there might be other cases when it's not possible to convert the destination parameter to a routed Url. This patch only uses the Url from the destination parameter to redirect if it's a routed Url, otherwise falls back to the entity canonical route.

keszthelyi → created an issue.

Patch from the MR.

keszthelyi → created an issue.

keszthelyi → made their first commit to this issue’s fork.

keszthelyi → made their first commit to this issue’s fork.

keszthelyi → made their first commit to this issue’s fork.

keszthelyi → made their first commit to this issue’s fork.

keszthelyi → made their first commit to this issue’s fork.

keszthelyi → made their first commit to this issue’s fork.

#31 is working for us, however there are two dpm() calls left in the patch. Here's the same patch without those dpm()s.

I updated entity_usage to 8.x-2.0-beta13 specifically to get the fix for this issue, because I also had the same problem.

The fix worked for the case when the source entity type didn't have any entities, however, I think it also caused a regression, because now entities with ID 0 or '0' are not passing the condition, and craching the batch process (causing an infinite loop). I opened a new issue about it: #3465800 🐛 Batch update hangs up if source entity ID is 0 Active

keszthelyi → created an issue.

This would need tests and confirmation if the issue reported is valid. Until then, posting a patch that fixes the problem described above.

keszthelyi → created an issue.

keszthelyi → created an issue.

This patch is almost identical to #8 which is created from @Matthijs' MR. The only difference is that I added support for plain text fields (to handle cases when the processed property is empty for the field).

@Matthijs I tested your MR on our project and so far it's working great, thank you! I need a fixed patch file because of our CI pipeline, so I upload a patch version from it (at commit d144a36e).

Currently, the module is not handling processed text correctly IMO, and this seems to have security implications also in certain cases, besides not applying the formatting. I'm planning to report that separately, but I will reference this issue (and your MR) as a possible solution.

I think that support for Text (plain, long) would be more important than Text (plain), which allows only limited number of characters and is for shorter texts (title). I created a patch for supporting both.

@maxilein, @weseze There is another issue and patch for the exception thrown when the entered value is too big: https://www.drupal.org/project/drupal/issues/1003692 🐛 PDOException: SQLSTATE[22003] after entering large value in integer and decimal field Needs work

keszthelyi → created an issue.

keszthelyi → created an issue.

DamienMcKenna → credited keszthelyi → .

@simohell yes, I'm aware of that, but our CI doesn't allow MR patches (not even on commits). I could have used a local patch, but I know there are others in the same situation, so maybe it will be useful for others.

Created a patch from the accepted solution for anyone, like me, who needs to use a fixed patch file.

Tested #5 and it fixes the issue, also it is the correct solution IMO:

1. for UniqueReferenceFieldValidator access should not be checked (see #4)
2. fixes another case where accessCheck is not set on the query in GroupPermissionsController::getRevisionIds() (where we should check the access)

This patch is identical to #136, only I removed one change introduced by this commit. I'm not sure if that permission check is needed (as view access on the parent comment is checked), and feels like it goes against the idea of the patch itself (to use access checks instead of permission checks). We use this patch together with Group Comment module, and that check would only allow replying to comments if we'd use the 'View comments' global permission, and we'd like to avoid that in favor of group permissions.

The tests would need to be fixed of course, but I'd rather wait for feedback on the idea itself first. Changing the status to Needs review for the feedback, it's obviously not complete.

I'm sharing this patch that we are planning to use for now, as it fixes the issues for our project. Not sure if this is the right approach, and it's possible that I misunderstood something about this cache context or about the way the hash is working. I also understand, that this could create a lot more variations for the cache context in cases, making it less effective, and this might not be ideal for all projects. The patch does not allow identical hashes for different users, unless they are members in the same set of groups.

keszthelyi → created an issue.

Patch from branch 8.x-2.x at commit 58439194

Patch from MR at commit 7cdc8c1d

Patch from branch 3254300-media-library at commit 3a4d570

Removed the string check from #3 as we can be sure it's a string if not NULL.

Patch from 77844c29

Patch from 3c71bdcd

keszthelyi → created an issue.

This patch is combining #3 and #5, in case you need a temp fix and also want to catch exceptions.

I can confirm the issue, and also that #3 is working at the moment and the thumbnails are retrieved with the URL fix. The fix is not ideal though, this should be fixed in the API, which returns the bad URL, or maybe in the request that retrieves the resource, not sure.

However, I think the request exception should be caught when trying to get thumbnail, so in that case the method should return NULL without exception. (Ideally, this should also be logged, but the fact that the thumbnail was not downloaded is logged in MediaAvPortalSourceBase::importRemoteThumbnail())

The attached patch is only for catching the exception (committed in 29c7af22)

keszthelyi → made their first commit to this issue’s fork.

@luzgallego Thank you for the patch!

Committed a constraint based solution to the issue fork (rebased the branch and changed the version to 2.x). Ideally, there should also be a test case for this. Attaching the patch version here.

keszthelyi → made their first commit to this issue’s fork.

Hi @joanpebupe, when requiring drupal/entity_meta_relation directly, then there's no entry in autoload_psr4.php and it's working by being loaded by Drupal. Yes, I'm planning to report this on oe_list_pages.

I also have the same issue, emr messing up the top level src directory custom project namespace, causing PHP Fatal error: Cannot declare class... errors for certain scripts.

I digged into the composer autoload generation, and after some debugging, I found that in my case it was not this module that caused the issue, but the fact that this module was a dependency for another module (OE Listing page). That module is requiring a metapackage called 'emr', that doesn't have an install path, because of this an empty string will be used by composer in the packageMap here, and that results in the 'Drupal\\emr\\' => array($baseDir . '/src'), in the autoload file.

Not sure in what context others have this problem, but after I found this, I installed the module alone (not as a dependency), and could not reproduce the issue anymore.

I ended up replacing the emr metapackage and requiring entity_meta_relation directly in my project's composer.json to be able to use the module.

Patch for fetching the high resolution photo thumbnail if possible.

keszthelyi → created an issue.

Hi @larowlan,

Yes, that's the issue I have. Thanks for pointing out that issue!

The issue happened to me today on Drupal 9.5.5 (PHP8.1). I changed permissions on 2 roles and the config export started numbering the permission array keys that previously were just listed with the dash sign (as expected).

Reroll of #2 for 8.x-1.0-beta5

Patch from commit 6352b64