Merge into Easybreadcrumb?

Hi Greg, yes it’s good idea
I’m interested on merging dynamic_breadcrumb on easy_breadcrumb

+1 for the merge.

Awesome. Since Dynamic crumbs uses breadcrumb alter and Easy Breadcrumb doesn't, we should be able to just copy and paste the code into EB and then just test to make sure everything works together nicely.

I've added you both with full permissions on Easy Breadcrumb.

Thank you Greg,
Okay i will try this when i have some time.
I guess we still need to add the config of dynamic breadcrumbs to the EB form, like the both modules will have the configuration in the same place.

Sounds like a plan. I will work on it as well when I can.

I just pushed first version of the merge in this branch
here is a draft MR https://git.drupalcode.org/project/easy_breadcrumb/-/merge_requests/99

Todo:

1. Maybe an Ajax button to update fieldSets of config when user check other entities
2. Move code from hook_system_breadcrumb_alter to EasyBreadcrumbBuilder, we need to adapt applies and build methods
3. Test that all features works together
4. Maybe rename configuration naming (value and checkbox not so clear)
5. Maybe add hook_update for people who has already the Dynamic breadcrumbs or block users to update to version that have already Dynamic breadcrumbs merged, and tell them to disable it and just update easy breadcrumbs it has already the DB

Number 2 is fixed here https://git.drupalcode.org/project/easy_breadcrumb/-/merge_requests/99/d...
I moved the code from the hook to the class maybe need some optimisations.
I let you review the MR.

I fixed Drupal CS and i added a hook to give developer the possibility to add their own route, this hook is useful when you have custom entities
https://git.drupalcode.org/project/easy_breadcrumb/-/blob/feature/megre_...

I hesitate about the alter maybe we don't need to give the possibility to alter allowed routes, maybe someone don't want dynamic breadcrumbs to be applied on taxonomy routes, but they can do it in configuration just donc check Taxonomy term entity type.
Anyway when you have time you can review the the MR.

Wow, code looks great! Adding extra hooks doesn't hurt, gives programmers the options to write custom code if they want to.

I think once we test everything, we're almost ready to merge.

There's some phpunit test fails we need to update first:

Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for easy_breadcrumb.settings with the following errors: easy_breadcrumb.settings:entity_types missing schema

Yes, you are right adding extra hooks always good 😉, itself good, the hook to add more allowed routes is mandatory otherwise the custom entities even if the dynamic breadcrumbs configured it will not work because it will not pass the if condition.
I was wondering just about the hook alter if it has really a value.

Phpunit tests should be fixed, and do more testing of the features, i did test but not all the cases.

I got you now. I think it has value. In fact, I was thinking tonight of making the routes a configuration item to make them customizable after someone made a new builder to customize crumbs on a single route:

https://www.drupal.org/project/easy_breadcrumb/issues/3424369 💬 Node with multiple taxonomies - can I pass value as URL param Active

Yes, it's a good idea to give user the ability to config or alter routes where they want to apply the builder.
I created this issue 📌 Due to the merge into Easy Breadcrumbs add hook_install Active to prevent both modules be installed in the same time after the merge.

I just committed https://git.drupalcode.org/project/dynamic_breadcrumb/-/commit/42220efd8...
I assumed that the merger will be in the next release 2.0.7, if it's not the case, we can change the version later just before create the new release of DB.

Still have hook update in the release of merge if the DB already installed, we should migrate the config to EB and uninstall DB module.

wow, super smart, didn't think of that.

I just pushed a commit to add hook update and into hook_install if the module DB installed migrate it's configuration and uninstall it
https://git.drupalcode.org/project/easy_breadcrumb/-/merge_requests/99/d...

I just fixed the conflicts and update the code in feature/megre_dynamic-breadcrumbs branch
@Greg Boggs Let me know when you have time to look at the MR.

I'll get it taken care of this week.

I tried to fix unit tests this commit, but still 3 errors to fix, and maybe add other tests for dynamic breadcrumbs.

@berramou can you bring up your MR to the latest dev Version... I guess the 3 failing unit tests should be fixed then... because those where in the main codebase

Thank you @spuky yes it fixed the 3 failing unit tests.
@Greg Boggs if you have some time to review the MR, and maybe add more tests for dynamic breadcrumb ?

@berramou you might want to open an issue in the EB issue queue. And attach your MR there so it gets more eyeballs and also it might be easier for folks to try it out. By just adding the diff as a patch to a composer.json. It is still on my todo list... to check it out closer... but i did not get around doing it.. noticed it has an merge conflict with the latest dev version...

Thank you @spuky, i created an issue ✨ Merge Dynamic breadcrumb Active