🇪🇸Spain @marcoscano

marcoscano → created an issue.

I agree with the 2-row suggestion above, I too think that displaying the paragraph chain somehow is useful information for editors.

As for the status/used in column, I think any solution that combines them will improve things, and the wording should in all cases refer to the top-level host entity, not the immediate parent entity.

marcoscano → created an issue.

@shashank5563 thank you for reporting a bug.

Please don't direct message maintainers of open source projects saying "Fix issue XYZ asap" because that type of approach discourages maintainers from volunteering their time to help you.

Bugs reported in this issue queue can be fixed by any community member by creating Merge Requests with the proposed solution. Providing fixes with test coverage included increases the chances of the fix getting committed sooner.

Thanks for your help.

👍 sounds good to me, I can't think of a downside. We just need test coverage for the new functionality.
Thanks!

Just tagged https://www.drupal.org/project/entity_usage/releases/8.x-2.0-beta25 → with this fix.
I was hopeful we could include other improvements in the same tag, but they are not ready yet. No big deal, I'll cut another release when that happens.
Thanks for your patience.

I agree that the root of the issue is that under the hood we track 1:1 all relationships without distinction, but on the UI we don't want to display all of them in the same way. This distinction makes the controller code complex, inefficient and prone to errors and edge cases.

That said, I agree there is a bug here we can fix to improve editorial experience of the scenario you describe. IMO the fix should be to:

1- Add the "Used in (Old revision's)" column also in your scenario 1
2- (unrelated, we can do this in a follow-up) Hide the "Status" column whenever the "Used in" column is displayed (or when the usage doesn't refer to the default revision of the host). To me it makes no sense to display "Published" or "Unpublished" for the host when the usage isn't in the default revision.

We may surface other edge cases by doing so, but I agree this is likely a step forward in improving things.

Thanks for working on this. I haven't reviewed the MR yet, but re-rolled it so we can test manually and see the results of the automated tests.

Functionally, I see that for each level of nesting, we are creating a new row in the usage table... Wouldn't that be confusing to editors?

@daddison thanks for reaching out. I plan to tag a new release later this week 👍

It seems like you opened 2 issues for the same problem?
Marking this one as duplicate of 💬 Can you extract these XPkeywords? Active

Can you try removing the module from the contrib directory, and running composer install again? It seems you have mixed versions of the module's code, either from git clone or other means... (also, make sure `/modules/contrib` is added to your `.gitignore` , since contrib modules shouldn't be added to git if you are using composer to manage the dependencies)

@theorichel it seems your image doesn't contain the metadata you are looking for.
You can verify all metadata this module can retrieve by putting a breakpoint inside the alter hook and inspecting the variable $raw_metadata (or by just dumping its value on the page if you don't have xdebug configured).
If what you are looking for isn't there, then either the image doesn't have it embedded, or it's using a standard different than IPTC / EXIF / XMP, which are the only ones this module supports.
(for the record, it seems the sample image you are using doesn't contain these metadata values, you can check by uploading it in one of the multiple online services that exist for testing images, such as https://getpmd.iptc.org/getiptcpmd.html or https://www.imgonline.com.ua/eng/exif-info.php )

Thanks for helping.
We no longer work with patches on drupal.org, the proposed change needs to be in a Merge Request, which will then trigger the testing pipelines.

Merged, thanks for helping!

I have rerolled with -dev and all tests are indeed green.
Thanks everyone for working on this!

Excellent, thanks for helping!

I recommend we keep 10.x compatiblity till 12.x is at beta and then we release a new version based off 8.x-2.x that is 11.3 or 11.4 and 12.x compatible and handle all the deprecations there.

Sounds good to me 👍

Thank you!

IMO this is a bug because it is breaking sites that extended those clases, and was introduced in a minor release.

Here is the error, for folks reaching this issue through Google:

Fatal error: Class Drupal\my_module\Form\BulkConfirmForm cannot extend final class Drupal\views_bulk_operations\Form\ConfirmAction in /var/lib/tugboat/web/modules/custom/my_module/src/Form/BulkConfirmForm.php on line 22

It seems these classes were marked final as part of 📌 Improve module code quality checks Active , which seems to have been CS fixes only? (instead of a real desire to make these classes non-extendable). If so, I suggest we remove the `final` declaration from these, and silence phpstan notices, if necessary, otherwise.

In any case, I see that the MR has all pipelines green, so we may not even need anything else here? Marking RTBC since it's good to go from my point of view.

Thanks!

marcoscano → created an issue.

I would like to opt-in this module: https://www.drupal.org/project/media_image_metadata →
It is a very recent project, it has 5 issues, and all maintainers agree we are ready to try GitLab issues. We understand we cannot revert this change and there may be some unexpected issues as early adopters

Thank you!

marcoscano → created an issue.

I may be missing something here, but media_image_metadata.extractor is not part of this module, so there's likely something wrong with your code. Feel free to re-open if you can reproduce on a clean install.

marcoscano → created an issue.

Thank you for filing this.
I looked into the modules, but I believe they solve different needs. This module's scope is to extract embedded metadata from images and provide them as media metadata attributes so they can be mapped into media fields.
I would like to keep the scope restricted to this, so we can make sure we do well this narrow set of features, without diversifying too much.

Thank you for your interest!
I have _just_ published this (yesterday!), so I don't necessarily need help with maintaining it right now :)
That said, I'm happy to consider new features if folks believe they make sense into the module. Feel free to open new issues and/or provide MRs with new functionality or bug fixes that you have already identified in the past.
I'm marking this as closed for now, but I'll get in touch if my availability / interests in maintaining this changes in the future.
Thanks again!

Your patch is only reverting what has been done in #3514883: Assertion fails when using external stream wrappers (e.g. s3fs) in PublicFileIntegration →
Unless I'm reading things very wrong, wouldn't this fix be re-introducing the bug fixed there?
I believe we first need to make sure we have test coverage for both bugs, and then come up with a fix that is compatible with both of them.

Thank you for creating the submodule.
For people looking for a standalone module, I have created a https://www.drupal.org/project/media_image_metadata → that also supports IPTC and XMP metadata, on top of EXIF. I have marked this module as being replaced by media_image_metadata in the project page.

Thank you for the pointer.
I have created a new module https://www.drupal.org/project/media_image_metadata → that also supports IPTC and XMP metadata, on top EXIF. I have marked this module as being replaced by media_image_metadata in the project page.

I have reviewed the MR and the essence of the changes look good to me. There are a few coding standards violations but I'll leave to maintainers to decide whether that's a blocker or not.

Also, I feel the batch size of 50000 items could be made configurable, not sure if that could still be too much in certain low-memory environments. This could be done by either by exposing it directly in the module's settings form, or just with a setting in admin_feedback.settings and sites could override the value in settings.php to a different value as needed. But again, not a blocker, and could be done in a follow-up as well.

Thank you for the proposal.
This is a very simple and lightweight module, where the underlying architecture is just to use JS to hide/show values that already come from the back-end. Doing what you describe is technically a very different solution, which IMO would go beyond this module's original purpose.

From what I see on the module page, the other module you describe is trying to fix exactly this? If that's the case, I'd rather we concentrate efforts to make that module work well, instead of replicating the new feature here?

Marking this as won't fix, but others feel free to keep commenting here if there is a lot of people that think differently.

Thanks!

Thanks for filing this.
Things would be so much easier if Media Entity Download routes were named entity.media.download or entity.media.media_entity_download.... Is there a way to do that in a backwards-compatible way? That would be my first option, if possible.
An alternative is to make that contrib compatible with Entity Usage ( ✨ Track the usage of Media entity through media_entity_download route with entity_usage Needs review ), instead of the other way around.
I am not opposed to this here, but it's always a balance between adding special-cases versus trying to limit maintenance burden over time...

I believe the issue here may be that with an entity_reference field we get some views integration for free, such as a relationship, whereas with an entity_reference_entity_modify field, that doesn't happen. For example, OOTB we can't add a relationship to the referenced media item in a view when using this module's field type.

Did you see https://www.drupal.org/project/entity_usage/releases/8.x-2.0-beta18 → ? In that version the regenerate queue was deprecated, any cron job or drush command using it needs to be manually disabled. You might just need to disable / remove that cron job from your Ultimate Cron configuration (I'm not really too familiar with how that module works)

Thanks for filing this and for proposing the fix. Since we don't have visual regression tests for this type of change, can you please provide before/after screenshots?
Thanks!

Thanks for working on this.
However, my original idea was that we could integrate with an Icon-provider, but not necessarily restrict icons + thumbnails to this solution. I can see how large projects might have design teams in-house where they are able to produce their own icons and thumbnails, so we'll want to keep supporting local paths to custom assets, and only offer to choose from an asset library as an opt-in.

Also I don't love the idea of having a hard-requirement on a contrib module for a feature that is not essential to the module, so that's why I suggested we added this functionality in a submodule, for example.

In any case, I haven't dug into too much detail, but it looks like the 2.x branch of https://www.drupal.org/project/iconify_icons/ → will indeed use core's icon API _and_ the UI Icons module to integrate with Iconify as icon provider? Maybe that's a good path forward then to have the best of both solutions?

Thanks for filing this.

In -beta18 we did clean up old hook implementations, the most recent one that was removed was a hook introduced in -beta9, so quite old...
Anyone using a version earlier than -beta9 will indeed need to upgrade to -beta17 first, and then bump to >= -beta18 in a subsequent deployment.

(That's why it's important to try to be relatively up-to-date with latest versions... )

In any case, it's true the release notes could indicate this, so I have just edited -beta18 release notes to include this:

Upgrading from older releases

In this release we remove old hook implementations from entity_usage.install, in order to simplify maintenance. If you are using a version older than 8.x-2.0-beta9, then you should update to 8.x-2.0-beta17 first, and then in a subsequent deployment update to this (or any release older than -beta18).

Thanks!

Thanks @mmenavas! Committed and tagged a new release → with the fix. 👍

Maybe the formatter settings can have this extra field where we could store the domain, since this is specific to this provider.

Thanks for reporting! Yes we shouldn't be altering all forms, only options-type fields. Can you please test the MR just to double-check before merging? Thanks!

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

Thanks @mmenavas! Looks great 👍
I went ahead and created https://www.drupal.org/project/url_friendly_options/releases/2.0.0 → after merging.

Thanks for raising this up, it had felt outside of my radar.
In recent work we have added support for redirect and as part of that we realized it didn't make much sense to expose redirects to be used as source but not track redirect basefields, since that's the only way redirects can be linked to other entities. Because of that now if redirects are selected as source/target, we disregard the option to track basefields and also track redirects since that's the intended most specific behavior.

I would be happy to move forward the same special-casing for menu links, since that's apparently the same scenario?

Support for redirects has been implemented recently in 💬 Entity Usage when a URL Redirect Exists Active and the UI displaying usages was improved in 📌 Consider updating usage when path aliases change Active . Note that changing a path alias / creating a redirect outside of the affected entity's form makes it challenging to update all places in the system that could be using the old URL. Due to this, when there is a need to re-calculate tracking based on a URL that changed, we use a queue to update the corresponding usage information. This means that the system won't be really "up-to-date" until the next time you run cron and all queued items have been processed. (This may explain why you see differences in behavior after you create the redirect and after you run drush to re-generate statistics... the second action would be equivalent of running cron (or waiting a bit if you have cron scheduled to execute in background)).

Does this clarify things a little bit?

I'm thinking that if we go the route of identifying Trash is enabled in the usage controller in 📌 Document interaction with the Trash module Active we will likely be able to fix this as part of that issue too?

@bkosborne sorry just re-read the issue summary and I am realizing you had already suggested something along those lines...

An alternative approach would be if this module detected if the Trash module was enabled, then it could deactivate trash's hiding feature on the usage page to it continues to show that usage, but then also indicates that the source entity is in the trash. All of that adds a pretty big coupling to the Trash module though.

So yes I believe we should go this route, even though I think it's suboptimal from a maintenance perspective... 🤷‍♂️

I looked a bit deeper and I see that there seems to be a way to execute code bypassing Trash's hiding mechanisms. I don't really love the idea of special-casing Trash's behavior in our controller, but at this point maybe it's a good compromise?
So instead of having:

        $source_entity = $type_storage->load($source_id);
        if (!$source_entity) {
          // If for some reason this record is broken, just skip it.
          continue;
        }

we could have something along the lines of (untested):

        // Special-case the Trash module behavior, which hides the entities in
        // trash from all entity_loads.
        if (\Drupal::getContainer()->has('trash.manager')) {
          $trash_manager = \Drupal::getContainer()->get('trash.manager');
          $source_entity = $trash_manager->executeInTrashContext('inactive', function () use ($type_storage, $source_id) {
            return $type_storage->load($source_id);
          });
        }
        else {
          $source_entity = $type_storage->load($source_id);
        }
        if (!$source_entity) {
          // If for some reason this record is broken, just skip it.
          continue;
        }

and similarly to other places we may be checking things on the source entities (for example access checks, getting the labels, etc). If we know the entity is in trash, we may even add a suffix of type " (in trash)" to the label...

Thoughts?

Fixed in 🐛 Exception thrown on usage page if a layout builder block has usage but its parent entity was deleted Active
Thanks!

Thanks for raising this, and for contributing the fix.
I agree on the approach, there are downsides as you mentioned but we are not making anything worse with this fix.
Also thanks for fixing 🐛 Entities used by content blocks should not use label or status of layout builder node unless they are "inline" (non-reusable) Active which I agree makes sense to fix here 👍

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

That's odd, I can't really reproduce it on a new installation. The first output you pasted in the issue summary made me think the code was being upgraded instead of installed for the first time.
Do you have anything particular about your project that may be interfering?

Thanks for working on this! I left a review, nothing super blocking, but it would be good to check if we can improve the test without the waiting calls.
Also, not sure if I'm seeing things right, but maybe the MR is being targeted against 1.x (instead of the 2.x new branch)
Thanks!

Ouch, thanks for raising this. It's indeed very unfortunate that the Trash module is so aggressive when hiding trashed entities... This means that our code in the controller:

        $source_entity = $type_storage->load($source_id);
        if (!$source_entity) {
          // If for some reason this record is broken, just skip it.
          continue;
        }

will just think the source entity is broken since it can't be loaded, and skip it entirely from the table. I actually think this behavior from the Trash module could produce other unintended effects on sites using that module, because "the entity was moved into the trash" doesn't necessarily mean IMO "no other piece of code should be able to load this entity".

I'd like to give this a little more thought, but at this point I'm leaning towards just displaying a general warning message at the top of the usage page if the Trash module is enabled, or something along those lines. Thoughts?

Good catch, this is indeed a bug.
Thanks for contributing!

After upgrading code to newer versions, you always need to run drush updatedb before doing anything else. Can you confirm if doing this after bumping to the new version fixes the issue for you?
Or alternatively, can you reproduce this in an installation where this module has never been enabled?
Thanks!

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

Sorry for the delay. This has been fixed in 🐛 InvalidArgumentException: The user-entered string must begin with a '/', '?', or '#'. Active

Sounds good to me, thanks for contributing! 👍

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

Yes, this would benefit all sites that are using nodes as some sort of "microcontent" that have no meaningful representation standalone.
I'm happy to move this forward if folks are willing to start a MR + tests.
Thanks!

Let's go with the identified fix for now, please open a new one with steps to reproduce if there is something else to be fixed here.
Thanks all!

Thanks for contributing!

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

Sorry for the late reply. It would be great if someone could submit this in the form of a GitLab MR, reroll with current -dev, and include the new #media template variable that was added in ✨ Add Media Entity To All Templates Active
Thanks!

Sorry for the late reply. It would be great if someone could submit this in the form of a GitLab MR, reroll with current -dev, and include the new #media template variable that was added in ✨ Add Media Entity To All Templates Active
Thanks!

I am not too familiar with videoask, but this PR is basically just allowing to embed anything from any URL, which defeats a little bit the purpose of the per-plugin providers architecture. If we could restrict the URLs accepted to something that's unique to this provider, it would be better.
Thanks!

Here too, let's clean up the code and include the new template variable added in ✨ Add Media Entity To All Templates Active , then this is good to go.
Thanks!

Sorry for the late reply.
I'm OK adding this provider, if you can please just clean up the theme variables, and make sure to include the new #media added in ✨ Add Media Entity To All Templates Active , we can get this in.
Thanks!

👍 Sounds good to me. Thanks for contributing!

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

Sorry for the late reply. Committed, thanks! 👍

I have spent some time looking into this today, and I too, am unable to reproduce it.
@wilco please update testing instructions on a fresh install if you can, or test if the MR provided solves the issue for your particular case.
Also, thanks @dave reid for helping out! 👍

Do you have redirects enabled as souce/target in the settings page?
As soon as you have a redirect in the middle it starts becoming one extra entity in the chain, so that needs to be tracked as a separate entity.

👍 nice! Thanks for helping!

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

I am not sure if we should avoid the empty string in the first place, but for now just replacing the !isset() with empty() solves the bug for me.

marcoscano → created an issue.

Hi @seanb! I am sorry this is causing issues for you.

Maybe @alexpott can chime in with more context if needed, but my recollection is that:
- The queue batch manager was a part of the codebase that had been added in the past as a stopgap solution to the poor performance when regenerating all info. It had been added without test coverage (sorry my bad), and had significant duplication with the normal batch manager
- In the past few months Alex worked on a lot of great improvements to the performance of the module in general, and in particular to the batch regeneration process

With the recent performance improvements (and knowing they are incompatible with how the queue works), it felt unnecessary to keep maintaining both batch managers, especially since they aren't particularly small/trivial. We felt the performance gains of the bulk insert batch manager were enough to justify having the Drush command always work as a standard batch operation, without the need to offer a queue mode as well.

Just tagged a new release with this and a couple other bug fixes.

Great, thanks! 🙏

Thanks for contributing!

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

Sounds good to me. Thanks for contributing!

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

@alexpott I am happy with the MR, but see the status is still NW. Did you want to include anything else or is this ready to go?
Thanks!

Thanks for contributing!
I have merged the PR, and tweaked the project page to include some end-user scenarios that include some of the terms you mentioned above.
I still think the relationship methods list is important since that may be very relevant to developers reading the project page, so they can know the extent of the possibilities of the module.

Thanks again!

Thanks for contributing!

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

You may be using a quite old version of the codebase. That variable no longer appears anywhere in `-beta23`.
Can you try with the most recent release and see if that's still an issue?

Thanks for pointing this out. True, the text on the project page is likely inherited from when the project was just an idea to expand what core's file_usage does... :)

Would you be up for creating a first pass at refactoring the page + readme file?

Thanks!

Sounds good to me. 👍
Clarifying the title though, since it was a bit misleading. The bug wasn't about the module tracking usage on types that were not set to, but it was collecting potential URL changes on all entities during presave, instead of collecting these URLs only on the ones that interest us (the ones marked as source). There was no functional bug, it was just a performance issue.