🇪🇸Spain @marcoscano

Thank you both! 🙏
Committed and tagged https://www.drupal.org/project/sam/releases/1.2.1 →

Thanks for working on this! I have not tested it myself, but I agree keeping it simple and adding the remove button only for widgets with more than one element makes sense.

RTBC to me, will wait a little bit in case @penyaskito has something additional to add since he reviewed the previous approach, but I'm 👍 to move forward with this.

Great work @penyaskito and @minirobot! I don't believe we need extra tests for now.
Merged and tagged 1.2.0 → with it.
Thanks! 🎉

Looks like Drupal CI is still running, would you mind turning it off now we have Gitlab CI we can save some minutes :)

Done! Thank you! 👍

Thanks for proposing this!

Are there many non-node content entities out there that are also created in a "standalone" workflow? (as opposed to an "inline" creation workflow, in the context of other content content edit form). I might not have come across this a lot in the past, so my first reaction is that this wouldn't be too common, in which case I'd rather this to either be a separate project, or sub-module, if possible.

marcoscano → created an issue.

Thanks for working on this! (and apologies for the delay in reviewing it)

I think this is very close. To me the only remaining issues are:

1- Add support for CKEditor5 textareas
2- Adjust the theming in Seven. (I know D10 no longer includes it, but there are a lot of D10 sites still using it in the contrib namespace, it would be uncool to break those sites... :) )

Thanks!

Apologies for the delay, but just making it official my support for this feature :)
Marking it NW as per the above reviews/comments.

👍

No, unfortunately this table is not a view, it's generated by code, and the columns are pre-defined. Sorry!

Adding related issues that cover part of the topic here, when we started this discussion in the realm of Media/File/Image fields and the confusion this generates:

🌱 [META] Once media is enabled, having the File, Image and Media reference fields all listed is confusing Active
#2930446: [PP-1] Improve field description texts for fields provided by core →

Also wanted to point out that a media field can be used for remote video content (a Youtube video for example). In this case, the "Upload" term isn't quite a good fit, IMO. On the other hand, some sites might have the option to store the videos "locally", instead of relying on a 3rd party service, in which case it would make sense to "upload" a video file into a media field.

Apologies for the delay, it makes sense, thank you!

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

Thank you! 👍

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

Thank you for reporting and for suggesting a fix. I haven't had a chance to look into the MR, but we do have a batch test ( \Drupal\Tests\entity_usage\FunctionalJavascript\BatchUpdateTest ), so it would be great if you could reproduce the failure in a test-only patch, and then we can use that as improved test coverage for this fix.
Thanks!

Thanks all who have been providing feedback and ideas to this issue. Apologies for not replying earlier 🙏

@acbramley thanks! I will take any help available :)

Currently I would say that both 3.x and 4.x branch are very much experimental and shouldn't be used on prod. Development on 3.x stalled at some point because I didn't feel good being the only one moving this idea forward (being this such a disruptive architectural change). Then at some point in time @seanb and @askibinski came up with the idea of splitting the API into a generic layer to "track things", and then make Entity Usage just be a consumer of that API, which makes sense to me, but we didn't fully make the switch into this new 4.x branch, and the development kind of stalled.

Yes, I think at this point it makes sense to envision the refactoring mentioned here on top of the 4.x branch. In order for that to happen, I would say that a rough roadmap could be:

ET = Entity Track
EU = Entity Usage

0- [NEEDS WORK] Fix tests in D10 / Switch to GitlabCI 📌 Fix tests in HEAD for D10 Active
1- [ALMOST DONE ?] review the current code / update the branches with latest commits on EU (entity_usage) 2.x and ensure we have feature parity between ET 1.x + EU 4.x and EU 2.x
- This was kind of OK as of Dec 2022 with #3324787: Update 4.x branch → and #3324797: Update with entity_usage 2.x changes → but we'd need to review latest bug-fixes since then.
2- [NEEDS REVIEW] ensure that the test coverage of ET 1.x and EU 4.x combined is equivalent of what we have in EU 2.x
- This probably happened as part of the above issues as well, but we'd need to double-check we are not losing test coverage in the switch
3- [NEEDS WORK] ensure we have an upgrade path for existing users on EU 2.x #3326110: Create an upgrade path for EU 2.x -> ET 1.x + EU 4.x →
4- [DONE ?] have some real world experience / feedback of ET 1.x + EU 4.x
- I know of one reasonably-sized project that is using ET+EU on prod for a couple years now, but it would be great to get more alpha testers out there if we can.

After this, I believe we could tag a EU 4.0.0-beta1 and mark it as recommended branch instead of 2.x.

Then, it would likely make sense to revisit the refactor from this issue and simplify everything in a 5.x branch probably?

I am OK going forward with this plan and welcome everyone that is able/willing to participate.

Thanks!

This seems to be enough in my preliminary testing.

marcoscano → created an issue.

Looks good to me! Thanks!

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

ckrina → credited marcoscano → .

Thank you for testing! I have committed and tagged beta2 → with this change.

👍 looks good to me, thanks!

longwave → credited marcoscano → .

marcoscano → created an issue.

Can't you use hook_entity_usage_block_tracking() for this?

I believe the answer is yes. In practice, to me the most important thing preventing us from having a stable version are the performance issues in some site configurations. There's a few different ways of approaching them:
- Work on the issues with the current architecture (I am skeptical that we can fix them in a non-BC way, TBH)
- Change the architecture completely as described in 📌 Refactor module architecture in a simpler, opinionated and more performant approach Needs review . This idea didn't have enough momentum to move forward and would be disruptive to existing sites, so I'm not sure that's a short term solution either
- Concentrate the efforts in creating a robust solution in core. There's interest in going this way, as described in 🌱 Add a reliable entity-usage system to core Active and 📌 Track media usage and present it to the site builder (in the media library, media view, on media deletion confirmation, etc.) Active , but it's still in very early stages.

That said, the beta status doesn't mean there's a fundamental issue with the module for most sites, there's a lot of sites using it in production out there.

First of all, I am 👍 to include this, thanks! (I too have been asked by editors "how do I empty an existing value?" :) )

I left a minor comment in the MR, doesn't look like we need too many changes IMO.

However, in testing this on Tugboat, I see a couple things we probably want to address:

1) In testing with the seven admin theme, the button displays uncentered:

would it be worth adding a little bit of CSS to try to make this look good(-ish) in all admin themes?

2) (the error appeared when revealing a bunch of values and then removing some of them), but now I can't reproduce it... 🤯 I need to go for now but I'll come back and try to reproduce it again.

Maybe we can add a few assertions to https://git.drupalcode.org/project/sam/-/blob/1.0.x/tests/src/Functional... to add test coverage for the new feature? That would be great.

Thanks!

@partyka thanks for working on this. The MR is still marked as Draft, can you please remove that when it's ready for review?
(also, there seems to be a lot of changes in the controller code that may not be related to this change? It would be great if we could remove those to make reviewing easier)
Thanks!

Just please let me know if, after considering this, you'd still like to have the generated row implementation in the controller.

Yes, I think this would be the preferred approach, for maintainability reasons at this point. Thanks! 👍

Thank you for creating this and for the MR.
I think it's OK to open an integration point and allow modules to alter the row contents. However, as mentioned above, I would rather have the current code do its thing and just dispatch the event (or even a good ol' alter hook, it works fine for me), so other modules can alter the contents of the generated row. I could see the most common use for this would be to tweak things a bit, not re-writing everything entirely. (maybe if projects need to do something completely different it would be easier to just take over the controller at that point?)

In my mind with this new hook/event there would be a small performance penalty to all sites, for the benefit of the ones that do need to alter something, but I believe the trade-off is worth it.

Thanks,

The MR itself is OK as is, but I do feel that before merging this we need to make sure:
- the views wizard plugins don't affect node/media items if they don't have trash configured on them, and
- document that these entity types are special-cased in these plugins. In other words, explain somewhere (maybe in a readme, or in the project page?) that if trash is enabled on node/media they will be automatically excluded from new views, but that won't happen with other entity types.

What do you think?

Thanks!

👍 sounds good. Closing this for now, we can always revisit this idea if a lot of people find themselves in the same situation over time.

Thanks for opening this issue and participating!

I would be tempted to say that if your site has just a few content types, maybe you could reconsider if this module is really needed?

From the module page:

This module helps sites with a large number of content types to improve the usability of the Content -> Add page

which is to say that this module started from the premise that we were dealing with a lot of content types that needed to be organized, categorized, etc. :)

In case you consider the module is useful for the other UI it provides, maybe you can rename the default category to something like "General" or some other wording that doesn't get too much in the way of your editors?

Sounds good, thanks all for contributing!

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

marcoscano → created an issue.

marcoscano → created an issue.

penyaskito → credited marcoscano → .

I hadn't realized the new class is symfony6+ only. This would probably mean we would need to cut a new major for this, which would be D10+ only.

Argh sorry messed up the file permissions.

Hey @sarwan I'm working on this for now :)
Thanks!

marcoscano → created an issue.

+++ b/src/Asset/AmpCssCollectionRenderer.php
@@ -164,10 +165,14 @@ class AmpCssCollectionRenderer extends CssCollectionRenderer {
+            \Drupal::messenger()->addWarning('Failed to load the css file');

I feel we need to come up with better wording for this, and maybe use a watchdog message instead of a message to the user?

In any case, the patch in #2 works for us when aggregation is enabled. I'm going to leave the ticket in "Needs review" so the maintainers can chime in on the above mentioned issue.

Thanks!

A few more.
(Sorry, this has been rolled (and checked) on top of patches from ✨ Execute node serialization inside new render context RTBC and 🐛 TypeError: Argument 2 passed to Drupal\applenews\ApplenewsManager::getMetadata() must be of the type string, null given RTBC so it might not apply to other projects as-is.

marcoscano → created an issue.

Thank you @mark_fullmer for the feedback.
I had a look at ✨ Show URL alias after autocomplete selection instead of internal route (e.g., /node/123) RTBC to see if it could be applied to our project, and unfortunately it doesn't meet our editors' current requirements (ie the alias isn't being saved in the markup, data attributes lost/removed, etc). Not sure if it's an issue with my setup or if I'm not testing things correctly, but for the time being, I'm just re-rolling the current patch on this issue. (I messed up with the re-roll in #9, this is a proper one against 6.0).

Thanks!

Closing for now, let's open follow-ups if we spot anything that needs further adjustments.

Re-roll on top of 6.0.0

Upgrade status was reporting an additional issue for me locally, updated patch attached.

Sounds good to me. Thanks for contributing!

Nice work! Thanks for contributing. I've added a link to the module on the project page, under "Modules that integrate with Type Tray".

Thanks!

Merged, thanks for the work and for the patience! :)

Good work, thanks! And sorry for the delayed review.

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

Makes sense to me. Thanks for contributing!

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

Looks good, thanks for contributing!

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

dan2k3k4 → credited marcoscano → .

Looks good to me. Thanks for contributing!

New release with this coming soon.

Thank you for contributing. Apart from the points I mentioned in the PR, we also need to:
- update the Readme.md file with the new provider
- update the media_remote.schema.yml definitions for the new provider settings

Thanks!

Sounds good, thanks all!
(and apologies for pulling the trigger too fast! :D )

Looks good to me, thanks for contributing!
My nitpicky brain has a tendency to complain that there's nothing guaranteeing that at the template level the {{ category }} variable will be CSS-safe (in theory it could be almost any string), but I believe the idea is that these would be unique, machine-names, and it's fairly safe to assume them as CSS ids, so let's go forward with this.
Thanks! 👍

+1 for closing this and relying on the mapping of fields to work as expected.

I agree though that many people won't know this immediately, and I am having a hard time trying to think of a good place to document this... The best I can think of is https://www.drupal.org/docs/8/core/modules/media/creating-and-configurin... → , but that too, isn't too discoverable. Another option could be some help text in the media type config form, but this is really only specific to File/Image media types...

This is still WIP, I couldn't test this in our project yet due to a few dependencies' issues, but this is a first step at changing the deprecations reported by upgrade_status.

marcoscano → created an issue.

Hi there! Thanks for using the module and providing feedback.

There is some background in 🌱 [META] Project Roadmap Active , along with the issues we considered mandatory for stable releases back then.

That said, at this point in time I would safe it is generally safe to use the 2.x beta release in production sites. The idea of refactoring the module didn't get too much traction, and it is currently on hold.

Looks good! Thanks for contributing!

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

👍 thanks for contributing!

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

I have tested this manually both in D9 and D10, and all looks good to me.
Attached a slightly modified patch, that doesn't change its nature. It should also fix the issues others reported above about the patch not being applied in the packaged version of the module (which includes some automated metadata to the .info file).
Credit still should go to the bot :P

Thanks!

All looks good here, I've manually tested these changes both in a D9 and D10 installation.
Just uploading the same patch with a few slight adjustments that shouldn't change its nature. Credit still goes to the bot :P

Thanks!

Turns out I was wrong suspecting from the long paths, that was my IDE playing a trick with me. Turns out the keys were fine and not trimmed at all.

On the other hand, I did find that sometimes (I'm guessing when the namespace pointed to a custom site-specific (in a multi-site) theme directory) we had a trailing slash, which made the str_replace() not do the job it was supposed to do.

Attached patch that seems to solve the issue for us.

Thanks for contributing! The fix makes sense to me, committed to -dev.

I came across 🐛 Templates not found in some theme directory configurations RTBC today when upgrading to 3.x. It doesn't seem to be an intentional breaking change, but something to be aware of, since it seems other sites are reporting similar errors.

marcoscano → created an issue.

Thanks for contributing!

I agree this would be a nice addition to the usage page, and I'm onboard with adding this to the module. I left a comment on the code about improving wording on the UI text, but apart from that I don't see big red flags.

It would be important to add test coverage to all new scenarios we are adding, combinations of languages, pending/past revisions, etc.

Thanks!

marcoscano → created an issue.

This reverts that specific change.

marcoscano → created an issue.

After re-reading the related issue I see that back in the day I expressed that this was a "hard problem" to solve in core but didn't expand too much on that.

I still think it's non-trivial, especially if we want a solution that would both 1) register the data, and 2) present the data in a meaningful manner to end users. To me the trickiest aspects are:
- Depending on the content model, there may be "intermediate entities" (eg paragraphs, inline blocks, etc) that don't have a standalone representation (in other words, they "don't mean anything on their own to end users"), but on a technical level they are just as important as any other entity. So we need to track them the same way because they are part of the chain, but possibly display them differently (for example omitting them) on the UI. This causes complexity and overhead in the code that display usages to users.
- In content models that have a very large relationship tree (nested entities), calculating usage on entity save is very expensive. In entity usage we introduced a mechanism to allow updating the usage table as a background process (using a @destructable service) but that introduces its own complexities and inaccuracies to handle in certain scenarios.

Having said that, I too face this need in almost every project I work on, so despite the challenges, I am +1 for trying to find the most reasonable way to have this type of functionality in core.

Adding 📌 Track media usage and present it to the site builder (in the media library, media view, on media deletion confirmation, etc.) Active since it has some background discussion that might be helpful here.

Thank you @webchick

👍 that makes sense, thanks for the feedback. As for clarifying the change in the notice, you mean just changing the text of [#3332350] , or also including a note in the @deprecated message? I added a few words here as an example.

Since the patch is quite trivial, I went ahead and generated it, but still interested in hearing from the maintainers on the preferred approach.