Instantiation of Listeners does not use ListenerConfiguration about import HOT 11 CLOSED

OK, thanks. No rush. I just pretty much finished the category import for our client and it now works like a charm (as far as I can see). I really like the M2IF. Thanks so much for creating it (and making it opensource). I did however find another problem, but I'll open another issue for that tomorrow. Have a good evening

from import.

@ktruehl I'll check that tomorrow, thanks for reporting.

from import.

Hi, i checked that, but i didn't think that the implementation is basically wrong, probably a bit confusing. As for example, the registration of a listeners should look like the following

{
  "magento-edition": "EE",
  "magento-version": "2.3.0",
  ...,
  "listeners" : [
    {
      "subject.artefact.process.success" : [
        "import_product_batch.listeners.clean.up.artefact"
      ]
    }
  ],
  databases: [],
  ...
}

which means, that the listeners are an assoziative array containing whereas each entry has the array out of listener dependency injection IDs. This should explain the prepareListeners() implementation

class EmitterFactory implements EmitterFactoryInterface
{

  ...

    /**
     * Prepare the listeners defined in the system configuration.
     *
     * @param \League\Event\EmitterInterface $emitter   The event emitter to prepare the listeners for
     * @param array                          $listeners The array with the listeners
     * @param string                         $eventName The actual event name
     *
     * @return void
     */
    protected function prepareListeners(EmitterInterface $emitter, array $listeners, $eventName = null)
    {

        // iterate over the array with listeners and prepare them
        foreach ($listeners as $key => $listener) {
            // we have to initialize the event name only on the first level
            if ($eventName == null) {
                $eventName = $key;
            }
            // query whether or not we've an subarray or not
            if (is_array($listener)) {
                $this->prepareListeners($emitter, $listener, $eventName);
            } else {
                $emitter->addListener($eventName, $this->container->get($listener));
            }
        }
    }

which recursively destructs the configuration array and adds the listeners to the emitter as well as the @Type("array") annotation in the TechDivision\Import\Configuration\Jms\Configuration implementation.

As our documentation has not been updated to the last version, i'm sorry for the circumstances and hope the the explanation above helps. As well i'll update the documentation as soon as possible.

from import.

The thing is that there is a configuration interface for listeners TechDivision\Import\Configuration\ListenerConfigurationInterface and that interface indicates that the listeners should be given as

  "listeners" : [
    {
      "id" : "client.import_category.listener.finalize.empty.attribute_value.purge",
      "event" : "subject.artefact.process.success"
    }
  ],

In the end I find a configuration interface for listeners more intuitive. In particular, if you have to go and reverse engineer the configuration format, as I had to. ;) But in any case, if you want to have key/value combinations, you should probably remove that interface (and the Implementation in the JMS config module), since it's only confusing otherwise.

Another point weighing in for the use of the configuration interface: This way you can have multiple listeners for the same event.

from import.

@ktruehl You're right, actually we're NOT using the Listener configuration class. I'll think about and refactor it, probably using the configuration interface as we for sure need support for multiple listeners for the same event.

from import.

I had a deeper look into the listeners now. As discussed above, the ListenerConfigurationInterface is not uses actually. I really don't understand why i've added it, probably it was a try and i forget to remove it. As M2IF uses the same structure in other cases like

{
   ...
   "observers": [
     {
       "pre-import": [
         "import_product.observer.pre.load.entity.id",
         "import_product_url_rewrite.observer.clear.url.rewrite",
         "import_product.observer.clear.product"
       ]
     }
   ]
  ...
}

and we use the listeners configuration already in some of our projects i would prefer to remove the ListenerConfigurationInterface as well as the Listener implementation itself at this time. I've already thought about changing the format of the observer configuration as well, so I'll probably switch to that structure in the next major release. Hope that is still o. k. for you?

Beside this, it is already possible to have multiple listeners for one event, e. g.

{
  "magento-edition": "EE",
  "magento-version": "2.3.0",
  ...,
  "listeners" : [
    {
      "subject.artefact.process.success" : [
        "your_library.listeners.one",
        "your_library.listeners.two",
        "your_library.listeners.three"
      ]
    }
  ],
  databases: [],
  ...
}

I've tested it and it works as expected. I've marked the issue with the enhancement label and will not close it until i've decided how to handle it in the next major release.

from import.

So, I needed a listener as some kind of post-observer hook, i.e. a method that is called after the last import row has been processed by an observer. Unfortunately the M2IF doesn't provide hooks like that without using listeners.

Can you please exactly describe where you expect the hook. I'm sure we can add it!

from import.

So. I removed the patches and changed the configuration as you suggested. Works like a charm. Thanks for the update.

Concerning the hook, I'm using it to reset attribute values. The M2IF does not remove attribute values that are given as empty in the import file. But if we want to use the import to continuously update categories (and products) we need this. To do this effectively, I'm running batches of deletes. So I need to be able to tell my observer "We're done. Take care of the remaining batches." That's why I need a kind of hook, for which I'm currently using a listener, which is not very pretty, since I need to compare instances to make sure that I'm calling the mop-up operation at the right time.

I went through the source code and tried to find a "natural" hook that I could use. But since you're using callback methods that are handed to the lexer, I couldn't find one. The optimal place that I can see is in TechDivision\Import\Subjects\AbstractSubject in the import() method right after

$this->getImportAdapter()->import(array($this, 'importRow'), $filename);

Only a post import hook is needed, since a pre import hook can easily be implemented using lazy loading.

from import.

Perfect, i'm happy to hear that :)

Regarding the empty attribute values you want to remove, i think M2IF already has a solution therefore. It should be enough if you add the attribute codes that has to be cleaned up in the params section of the subject like

{
  ... ,
  {
    "id": "import_category_ee.subject.bunch",
    "identifier": "files",
    "prefix": "category-import",
    "params" : [
      {
        "copy-images" : false,
        "clean-up-empty-columns" : [ 
          "<attribute-code-to-clean-up-1>", 
          "<attribute-code-to-clean-up-2>",
           ...
        ],
      }
    ],
    ...
  }
}

Did you try that?

from import.

Damn! No, I didn't, since in the documentation it says

Columns that doesn't contain a value are ignored by default. This means, it is NOT possible to delete or override an existing value with an empty value. To delete an existing value, the whole category has to be removed by running an import with the delete operation. After that, the category with the new values can be imported by running an add-update operation.

which is pretty definitive. ;)

Hm. I'll have to look into how this is working. But I imagine that it's not deleting the empty values in batches, so my solution might at least have the advantage of being more performant. For categories this is less relevant, but our client has tens of thousands of products and we need the import to run daily and take as little time as possible.

from import.

@ktruehl Is this issue still active or can I close it?

from import.