🇪🇸Spain @marcoscano

Thanks for contributing!

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

If you end up implementing it as a block based on Type Tray's categories, I think a submodule like type_tray_block or something like that makes total sense! 👍

Thanks!

and we moved all files and placed in site/oursite/files/{file_name]

Are the files managed in Drupal or these are just links pointing to files in disk that happen to be at these locations? If the files are not file entities in Drupal (ie in the file_managed table), then this module won't track them, by design.

Happy to get this in. Does anybody feel like proposing a Merge Request?
(just clarifying that the proposed resolution in the ticket description has it backwards... :) if there is a field on the node that can reference media entities, then the node is the source, and media is the target. In other words, the field is always attached to the source, and what users reference is always the target)

Thanks!

Thanks for filing this and for the fix.
Is there a scenario where old URLs should be supported too? In other words, should be change the regex to the new format or should we just accept the new format as well?

Fixed, thanks!

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

+1 for removing. Folks that know how to fix it probably don't need the reminder.

dan2k3k4 → credited marcoscano → .

I agree that the module could handle the misconfiguration in a more graceful way. Merge Requests are welcome if anyone feels like working on it.

Looks good to me! Thanks for the quick fix!

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

Can we please open a new ticket for this and create the MR there?
@justcaldwell it would be great if you could compare the current fix with this new proposal and see if it solves the performance impact you are experiencing? Thanks!

Looks good to me, thanks for contributing! 👍

marcoscano → changed the visibility of the branch 1.0.x to hidden.

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

Looks good to me, thanks for contributing! 👍

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

Looks good to me, thank you for contributing!

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

Thanks! :)

Thank you both, I think the approach that casts the variable to a string is simpler and more readable.

Thanks for contributing!

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

Thank you for fixing the (unrelated) test failures! 🙏

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

Just posted on https://www.drupal.org/project/media_remote/issues/3431910#comment-15753535 📌 Automated Drupal 11 compatibility fixes for media_remote Needs work . Unfortunately there are tests failing, we need to fix them prior to merging the D11 MR. Thanks!

We still need to fix the failing tests

Thank you for offering!
I believe the module doesn't need a new maintainer at this point. However, help from the community is always welcome.
Ways to help include:
- Help triaging issues / improving issue description with steps to reproduce, etc
- Reviewing patches / converting patches to Merge Requests
- Adding test coverage

I will do my best to avoid issues stalling when maintainer input is necessary.

Thanks again!

Thank you for offering!
I believe the module doesn't need a new maintainer at this point. However, help from the community is always welcome.
Ways to help include:
- Help triaging issues / improving issue description with steps to reproduce, etc
- Reviewing patches / converting patches to Merge Requests
- Adding test coverage

I will do my best to avoid issues stalling when maintainer input is necessary.

Thanks again!

Fixed using the second approach. Thanks! 👍

Never mind, it seems we do have another issue where this is proposed with test coverage, so I'm getting that one in: ✨ Track reusable Custom Block referenced by Layout Builder Fixed

Apologies for taking so long in reviewing this.
Thanks for contributing!

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

I'm OK with this, but I'd be happier if we waited until that patch lands in core, so we can add this patch here with its corresponding test coverage.

Thanks for helping modernize the module's codebase. I don't feel strongly about the readonly property and/or docblocks, so I'd say let's get this in and iterate over in 📌 Fix the issues reported by phpcs Needs review , where we can fix these and other CS violations that have bubbled up lately.

Thanks again!

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

Thank you all! I picked the last one and added it to the repo 👍

Thank you for working on this!

I wonder if there should be an arrowhead on one end?

I agree with this, if we could make the line an arrow, it may be more explicit the source->target hierarchy?

Thanks!

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

Thanks for contributing!

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

dww → credited marcoscano → .

Thank you for working on this! 🙏
I agree it would be ideal to have a visual representation of the "hierarchy" that the relationship represents: there is always a "source" entity and a "target" entity, these relationships are not bi-directional. In other words, this module tracks "what entity (or entities) is this entity pointing to". Do you think it's possible to iterate and try to capture that?
Thanks again!

hestenet → credited marcoscano → .

Thanks for filing this in! I am +1 with this idea, however I have 0 skills in designing or producing a logo. If anyone wants to contribute, I will gladly add it to the repo.

Thanks!

Thanks for contributing!

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