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

Merge Requests

Recent comments

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

Looks good, thanks for helping!

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

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

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

I'll be looking into this shortly
(this is in fact just a test comment for our RSS feed integration :) )

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

That's fine πŸ‘

My question was probably because from memory, the only projects where I recall non-node entities having their own standalone page were commerce projects. Most scenarios of other custom entities I remember were components of the page, in which case a type-tray-style "Add..." page wouldn't make too much sense (because components are normally more easily created inline, in the context of their parent page). That said, maybe this is a fair request just to support commerce sites, if nothing else... :)

Also, as long as the new entity type implements ThirdPartySettingsInterface, it should be possible to abstract our controller and other relevant code to be agnostic of the type of the entity we should affect. So it would be fine to refactor the existing implementation to be generic, if we can do that without too much work πŸ‘

I could see a new global setting with checkboxes along the lines of "Entity Types to apply Type Tray", with only "Node" selected by default, where admins could opt in other entity types, does that align with what you had in mind?

Thanks again!

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

I agree this is a good idea, at least for the grid view, where I think just trying to adjust the CSS to have a closer look to what core provides would be beneficial.

For the list view, maybe someone with design expertise could possibly help us come up with a solution that is aligned with the grid view, but accomodates both the thumbnail and a possible longer description text as well.

Thanks!

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

@kaszarobert thank you for offering to help. I have just added you as maintainer. Feel free to work on issues, commit fixes and tag releases as you see appropriate. I will still try to keep an eye from time to time, but realistically, that may not happen as often as I'd like so feel free to move forward as needed.

Thanks again, and welcome aboard! πŸ‘

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

Today I saw https://www.drupal.org/project/iconify_icons/ β†’ , which provides integration with a library of open-source icons. Here's an example when searching for "events" : https://icon-sets.iconify.design/?query=event

Maybe we could have a submodule extending the iconify_icons API and allowing people to select their Type Tray icons directly from the UI?

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

If the user does not have access to the content overview page, this link is still shown

I don't think this would be a very common scenario, since I believe most sites allow editors to go to a place where they can see all content they have created? I have a hard time picturing editors with permissions to create content but not to see the list of content of that type. That said, I believe it's a good idea to not print 403 links if we can avoid. Let's just omit the link if the user doesn't have access to the admin content route.

Additionally it would be nice to change this url in the settings

On this one I am of the opinion that it's not worth adding the complexity for the few times this would be needed. It seems to me a legitimate scenario for custom code / preprocess tweaks, as you mention.

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

It would be great it you could point out what needs updating, more specifically?
Even better, feel free to give it a try and improve it if you can!
Thanks!

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

I was kind of able to reproduce this by installing the Minimal profile, then applying a recipe that enabled gin + gin_toolbar + navigation (and imported config '*' from the navigation module).

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

Looks good to me! Thanks for contributing!

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

Thanks for opening this and for the suggestion.
I myself have no opinion since both options look good to my back-end eyes :)
But I have asked internally a few designer colleagues (crediting them in the issue btw), and the predominant feedback is that smaller icons do look better, so let's do it! πŸ‘

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

Let's remove the inline comment that becomes obsolete with this change.

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

Thanks for opening this, and for the MR!

Just adding some context, back in the day I believe the idea was that these icons were to be considered "shipped files" rather than "uploaded files" (there is more reasoning on the challenges behind this here β†’ ).

That said, I am OK using the FileUrlGenerator helper, because it seems to work with shipped files too when provided a relative URL.

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

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

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

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.

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

Great work @penyaskito and @minirobot! I don't believe we need extra tests for now.
Merged and tagged 1.2.0 β†’ with it.
Thanks! πŸŽ‰

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

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

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

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.

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

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!

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

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

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

πŸ‘

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

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

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

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.

πŸ‡ͺπŸ‡Έ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!

Production build 0.69.0 2024