🇫🇷France @louis-cuny

Merged and released
Thank you for contribution and reviews

From my point of view, this module is about making synchronization of (structure & more?) entities through drupal configuration system.
I would avoid mentionning taxonomies, blocks and menus in the logo and keep a symbol referring to entities instead of each one

The overall logo is looking good

louis-cuny → created an issue.

louis-cuny → created an issue.

I updated view_custom_table module to 2.0.6-beta3 on my project and it solved the issue. Error disapeared and module related usage is still working

Has already being fixed with https://git.drupalcode.org/project/view_custom_table/-/commit/c3e7916a36...
Sorry for the noise

Any chance to push a new release tag?

This other module could fix the schema by changing from sequence to mapping type
https://www.drupal.org/project/all_entity_preview/issues/3462656 🐛 Use `mapping` instead of `sequence` in `preview.schema.yml` Needs review

louis-cuny → created an issue.

Any chance to Merge this soon ?
I will have a project with kafka incoming in some time and would be happy to be able to use this module

I added the missing "/11" in composer
MR is probably ready for merge I guess

Please review latest MR

MR looks fine to me with psr/log accepting ^3.0

Is anything missing?

The MR on this issue is about removing the install/config file

About psr/log dependency, the MR is this one: https://git.drupalcode.org/project/kafka/-/merge_requests/2
From issue https://www.drupal.org/project/kafka/issues/3318381 📌 Bump ext-rdkafka dependency for PHP 8.1 and D10 compatibility Needs work

I open a new MR showing the correct changes

I don't understand why the MR is blank
but https://git.drupalcode.org/issue/kafka-3393309/-/compare/8.x-1.x...8.x-1.x

Entities are exported to config system (in database)

To retrieve the config in the config directory, you need to drush cex like usual

In my case, I found memory_limit issues in apache logs
Setting to memory_limit = -1 is fixing the issue for example

Fixed in the meantime
Thanks

louis-cuny → created an issue.

Thank you @mlncn

I added a "Documentation" link in the Ressource section

There is already a uninstall hook deleting the config

Import modes need to be redefined for each entity type

louis-cuny → changed the visibility of the branch 3393309-remove-legacy-configinstall to hidden.

Yes, because uninstalling the module doesn't delete the related configuration

I guess it could be a good add-on

Remaining work would be

- Add a hook_uninstall deleting module related configuration

How to handle this issue without creating BC breaks/new major version?

Maybe something like:
On any export, export to the new splitted configuration(s) and delete the corresponding part from legacy config
On import, for each entity type, if we find data on splitted config, use this one, if not, try the legacy config file

We need to make sure to handle entity types separatly.
Users may use structure_sync for all entity type but export only one time for some reason and wouldn't expect entities disapearing

We should then deprecate the usage of the old structure_sync.data config and plane to delete it in a next major version

~~~

Other possibility would be to work on a new major version where we simply ask to users to re-export everything after major version update

I tried a couple of years ago to work on this issue, but I faced issues and erased my work.
I was too ambitious and was believing in a v3 of this module.

Today, I think the strength of this module is to be lightweight and precise. Of course, it should be flexible and evolve to make it better, but never trying to be the one solution fits all.

With this in mind, I would suggest splitting the work in those steps:
1. Defining what should be common for any entity type handled by this module
2. Creating the plugin manager system and refactoring the current mechanism without creating BC breaks

And good to have would be
3. Add tests about adding a dummy entity type handling
4. Add documentation for how to extend this module with a custom Plugin/EntityType

About point 1. I would say we can make generic
- the export method
- the 3 types of imports (safe, full, force) at least as a first step (maybe later let Plugins implement any type of import)

But already facing an issue, the config schema is not extendable. I mean, there is only one structure_sync.schema.yml for all types.
As a first step, we could let plugins write to a different config with defining their own config schema
But later we should target this other important issue https://www.drupal.org/project/structure_sync/issues/3012034 📌 Split structure_sync.data.yml to reduce risk of conflicts Active , sadly hard to make it work without BC breaks.

Please feel free to challenge this vision and debate

Thank you so much for your contributions.
You are bringing good vibes to this project !

I reviewed the MR and changes, tests are passing, I merge

Thank you for your contribution

I agree with everything you have said
I would say it's missing testing empty and non-empty option field value

This issue is about tweaking the fields to be exported/imported
https://www.drupal.org/project/structure_sync/issues/3085904 ✨ Move import/export logic into plugin Active is about rewriting the whole module using drupal's plugin system as a base to be able to support other entity types.

For this issue, I would agree it would be nicer to use drupal's Plugin system instead of hooks
But the approach here feels ok with hooks at least as a first step

From my point of view, the export/import all available fields approach would not fit this module, it should probably remain light and precise

If others share this point of view (open for debate), I would suggest these next steps:
- Add tests
- Review and test manually
- Add something about this feature in the readme.md
- Opening other (related) issues to cover blocks and taxonomy terms
- (optionnaly, converting the hooks to plugin managers)

Thank you for your contribution

Almost good about this one, I reviewed the changes for now
Will test it later

#6 Worked like a charm on our production running project
Additionally, I reviewed the code that is looking good to me

Thank you for your contribution

Isn't this issue 📌 Stabilize FunctionalJavascript testing AJAX: add ::assertExpectedAjaxRequest() Fixed fixing this error in drupal 10.2+ ?

An issue exist about drupal core but seems to be planned for D11

Using the related work on production project for a while now, changes are easily readable

RTBC

I agree, we still need to make it more obvious about the differences between safe, full and force mode for taxonomies before we can merge this patch

Thanks

Thanks

louis-cuny → made their first commit to this issue’s fork.

Thanks

louis-cuny → made their first commit to this issue’s fork.

Thanks

louis-cuny → made their first commit to this issue’s fork.

@salientknight
Yes
It's the design choice of this module to use configurations

You can export your configs in Back office Config/Development/Export Config
Or with drush cex

@ccarnnia
Could you use the Merge Request tools for your patch please ? It will be easier for us to review

@salientknight
Yes
It's the design choice of this module to use configurations

You can export your configs in Back office Config/Development/Export Config
Or with drush cex

I would be happy to merge this issue, I totally agree we need to be able to retrieve tid that are currently wrong and broking relations

But what does happen if there is a term already existing with a tid that is going to be imported ?

We need a clarification of what is happenning in any case

We need to find a way to support any field provided by any module that extend our menus/taxo/blocks

Ideas are welcome

Users are aware this module is not obvious to use. Discussion about documenattion improvment are already ongoing in other tasks

louis-cuny → created an issue.

I rerolled the patch but removed the non-related to this issue stuff
Please review

Patch doesn't apply anymore

louis-cuny → created an issue.

Adding the settings could also be done before composer require, maybe it would avoid any confusion
I guess some users are still going to try to install and enable it before writting the settings
Maybe we should do some check to avoid the installation if settings are breaking the module

Actually, I don't understand the config in kafka/config/install.
It seems to not be used at all in the module. Can you confirm please?

If so, it's not related to the error, it would mean than only settings are used. But in the readme it says that settings should be set up after module installation.

I will try to restart the installation later

I suspect also something is wrong with composer requirements. Because of the current rdkafka ^5 requirement, I had to use --ignore-platform-reqs on installation (I'm using 6.0.3)

I was having an array of array for producer broker list

The related issue made me do this syntax error

Sadly not so easy to debug without xdebug getting in rdkafka
I found more logs adding $conf->set('debug', 'all'); in addition to $conf->set('log_level', strval(LOG_INFO));

louis-cuny → created an issue.

I think my installation has another issue
There is something strange, in ClientFactory.php for example, it's requiering a class Producer

use RdKafka\Producer;

Like if it would be some other composer package required. There is no package listed in the composer.json of the module
Or it's just Phpstorm that doesn't see php-extension classes ?

Some info about rdkafka extension I have installed:
rdkafka support enabled
version 6.0.3
build date Oct 5 2023 13:43:25
librdkafka version (runtime) 1.6.0
librdkafka version (build) 1.6.0.255

PHP Version 8.1.22

If I'm correct, these informations from phpinfo page are saying I have required librdkafka and rdkafka php extension ?

I am confused about librdkafka that is no more https://github.com/edenhill/librdkafka
But https://github.com/confluentinc/librdkafka. I guess it's the same, but no php client

I got some precisions

I'm using -beta1 version + https://www.drupal.org/project/kafka/issues/3318381 📌 Bump ext-rdkafka dependency for PHP 8.1 and D10 compatibility Needs work patch
I understand I need to update my configurations.

This issue is more about having this error so soon in the installation. I guess it would be better to let users install the module with the default configuration proposed in config/install
And later throw errors or show messages in status page for example

This happens on "drush en kafka", so the default configuration should be correct. I usually set the configurations after the modules are installed

I didn't do anything else than composer require and drush en (excepting insatlling rdkafka)

louis-cuny → created an issue.

Thank you, I will review your patch

The exported config did export correctly to structure_sync.data.yml
When importing in staging, with drush cim you are only importing the config, not the actual blocks, taxonomies etc

You need additionnaly to drush ib for blocks for example

This is not a bug, I close the issue

It is exporting from entities to configuration
You have to export your configuration to be able to commit the changes etc

drush @site cex

Yes, sorry if it's not clear

Duplicate of https://www.drupal.org/project/structure_sync/issues/3317768 ✨ Multilingual Support for Taxonomies Needs review

From 47.87kB to 13.72kB using tinypng
Please review

louis-cuny → created an issue.

I did the same test as @andrew.farquharson without the patch and it did work fine, maybe the issue is more precise than that but anyway. I'm merging because it is not breaking anything

The class StructureSyncHelper.php should be defined as a service like FieldSettingHelper is.

Yes it should, but we don't want to rewrite to much stuff to avoid breaking other issues patches
So, if @tostinni's approach is working fine. I am okay with it.

We will be able to use correct service declaration in a v3 of this module

The patch I created didn't work like I expected, I cannot remember exactly what was not working but their was some bugs

Thank you for your contribution !

Could you make some adjustments please :
- dependency injection instead of \Drupal::menuTree()
- camelCase for variables instead of snake_case ex: $flat_menu_tree
- use strict comparision for if ($menuLink->uuid() == $uuid) {

Sorry I forgot to update releases
please use 2.0.5 and it will work fine with D10

The log successes setting was not saved, not used and not showed in the form.
I fixed all that

Please review

louis-cuny → made their first commit to this issue’s fork.

Can you confirm your are using 2.0.4 please ?

I created a new release 2.0.5 with the fix

This issue as already been fixed
See https://www.drupal.org/project/structure_sync/issues/3330219 🐛 Fatal error on D10: Entity queries must explicitly set access Fixed

louis-cuny → made their first commit to this issue’s fork.

louis-cuny → created an issue.

#4 Worked for me too !

#55 works for me if I disable reference validation :

    $form['contact'] = [
      '#type' => 'entity_autocomplete',
      '#tags' => TRUE,
      '#target_type' => 'node,user',
      '#validate_reference' => FALSE,
      ....
];

Patches that are suggesting using dynamic_entity_reference does not feel right

I put it pack to needs work because of the broken reference validation