Barcelona, Spain
Account created on 26 April 2011, almost 13 years ago
#

Recent comments

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

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

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

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

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

Thank you! πŸ‘

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

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

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

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!

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

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!

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

This seems to be enough in my preliminary testing.

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

marcoscano β†’ created an issue.

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

Looks good to me! Thanks!

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

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

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain
πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

Thank you for testing! I have committed and tagged beta2 β†’ with this change.

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

πŸ‘ looks good to me, thanks!

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

marcoscano β†’ created an issue.

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

Can't you use hook_entity_usage_block_tracking() for this?

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

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.

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

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!

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

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

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

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! πŸ‘

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

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,

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

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?

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

Thanks!

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

πŸ‘ 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.

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

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?

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain
πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

Sounds good, thanks all for contributing!

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

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

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

marcoscano β†’ created an issue.

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain
πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain
πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain
πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

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.

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

Argh sorry messed up the file permissions.

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

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

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

marcoscano β†’ created an issue.

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain
+++ 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!

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

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.

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain
πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

marcoscano β†’ created an issue.

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

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!

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

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

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain
πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

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

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

Sounds good to me. Thanks for contributing!

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

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!

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

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

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

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

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

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

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

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

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

Looks good to me. Thanks for contributing!

New release with this coming soon.

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

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!

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

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

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

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! πŸ‘

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

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

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

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.

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

marcoscano β†’ created an issue.

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

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.

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

πŸ‘ thanks for contributing!

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

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

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

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!

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

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!

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

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.

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

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

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

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.

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

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!

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

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.

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

Thank you @webchick

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

πŸ‘ 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.

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

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

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

I can work on a patch, but I'm not sure what the maintainers would prefer... should we remove the type hint and allow metatag_generate_entity_all_tags() to grab de entity from the route, or should we think of something else?

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

Thank you all for reporting and for the fix. The fix seems to be simple enough that I'm OK getting it in even if we don't know what's causing it. I did change it into a more old-style check since we still support sites on php7.4, if I remember well.

Can you all please test with latest -dev to see if this is really fixed for you?

Thanks!

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

Hi there, I know this module has a fair usage of sites already using it in production (and I use it myself), but unfortunately it isn't ready for a stable release, in my opinion. There are a number of scenarios where there could be significant performance considerations that we should address before that (for example in πŸ“Œ Refactor module architecture in a simpler, opinionated and more performant approach Needs review I listed some of them). I hope you can find a solution for your current project.

Thanks!

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

Hi all, thanks for opening this ticket.
I have just triggered a test execution with PHP 8.1, MySQL 8, and Drupal 10.0.7, and all automated tests included in the module passed: https://www.drupal.org/pift-ci-job/2651702 β†’

It would be great to be able to see more information on the failing scenario. Can any of you try to get to the failing scenario, then check the Drupal log messages, and see if there are any further information there? The AJAX error indicated above does suggest there's something going on, but it doesn't give us enough information about what the actual problem is, so it would be good to get that either from the logs or from the browser console.

Thanks!

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

>FYI, it seems, that you accidentally pushed the "3222522-drupal-9-compatible" to the main repository! Just a heads up :)

True, fixed now. Thanks!

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

Yep, so the tests in 10.x failed: https://www.drupal.org/pift-ci-job/2646940 β†’

Let's work on fixing them here, targeting the now open 2.x branch

πŸ‡ͺπŸ‡ΈSpain marcoscano Barcelona, Spain

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

Production build https://api.contrib.social 0.61.6-2-g546bc20