🇪🇸Spain @marcoscano

@silvi.addweb thanks for trying to help, but opening a MR with the same changes of the patch, when in the previous comment I mentioned what was missing in the patch, isn't enough to move this to RTBC

Apologies for not looking sooner into this. We'll need test coverage for the new feature we are introducing.

Thanks for opening and working on a fix.
I would have preferred that we add test coverage for the new scenario we are supporting, but it's fine as-is, I'm getting the fix in in the meantime.

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

Fixed, thanks!

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

Setting NW as per #3

OK, all green, I'm merging this.
Thanks again for working on this!

Fixed, thanks!

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

Thank you for working on this! 🙏
I agree on the testing strategy you mentioned. Just removed the next minor/major and added the scheduled pipeline to be run weekly (thanks for the suggestion).
However, this run now failed, with error:

There was 1 failure:
    
    1) Drupal\Tests\entity_usage\Functional\Update\UpdateTest::testUpdate8206
    Failed to run installer database tasks: Database drupal not found. The
    server reports the following message when attempting to create the
    database: SQLSTATE[HY000]: General error: 1007 Can't create database
    'drupal'; database exists., Failed to CREATE a test table on your database
    server with the command CREATE TABLE {drupal_install_test} (id int NOT NULL
    PRIMARY KEY). The server reports the following message: SQLSTATE[3D000]:
    Invalid catalog name: 1046 No database selected: CREATE TABLE
    "test56035940drupal_install_test" (id int NOT NULL PRIMARY KEY); Array
    (
    )
    .Are you sure the configured username has the necessary permissions to
    create tables in the database?
    
    /builds/project/entity_usage/web/core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBase.php:226
    /builds/project/entity_usage/web/core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBase.php:141
    /builds/project/entity_usage/web/core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBase.php:113
    /builds/project/entity_usage/web/core/tests/Drupal/Tests/BrowserTestBase.php:367
    /builds/project/entity_usage/web/core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBase.php:92
    /builds/project/entity_usage/tests/src/Functional/Update/UpdateTest.php:41
    /builds/project/entity_usage/vendor/phpunit/phpunit/src/Framework/TestResult.php:729
    
    FAILURES!
    Tests: 1, Assertions: 1, Failures: 1.
---- Drupal\Tests\entity_usage\Functional\EntityUsageLayoutBuilderTest ----

Does this ring any bell for you? Has anything changed in CI between yesterday and today? :) 🤔

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

While allowed, it's a best practice to avoid underscores in URLs. There's plenty of documentation on the internet on the reasons for that, I believe the one more widely accepted is that Google won't parse words separated by underscores, while it will if they are separated by hyphens.

If that's important to you, you can always bypass the validation with the hook and implement your own validation that allows underscores...

This is done but it's not ideal to have to patch a dev-dependency. I have opened 📌 Remove lenient treatment from paragraphs as dev-dependency in CI Active as a follow-up so we can clean things up once paragraphs is D11-ready.

marcoscano → created an issue.

Looks good, thanks for helping!

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

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

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!

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!

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

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?

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.

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!

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

Looks good to me! Thanks for contributing!

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

Tagged https://www.drupal.org/project/type_tray/releases/1.2.6 → with the change.

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

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.

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

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.