Code cleanup

• General code cleanup
• Avoid duplicated code in pull form

The code refactoring will go in #3157110: Additional/refactor tests for the new architecture

Thanks for the patch. Will give a look when possible.

Triggering automated tests.

1. +++ b/modules/entity_share_async/src/Plugin/QueueWorker/EntityShareAsyncWorker.php
   @@ -89,17 +88,7 @@ class EntityShareAsyncWorker extends QueueWorkerBase implements ContainerFactory
   +    $prepared_url = $this->importService->prepareUuidsFilteredUrl($url, [$item['uuid']]);
   

   Nice. Can we make this method a static one in EntityShareUtility.

   Because it is only manipulating data on its own without calling anything else.

2. +++ b/modules/entity_share_client/src/Service/StateInformation.php
   @@ -191,6 +143,62 @@ class StateInformation implements StateInformationInterface {
   +      'local_entity_link' => $entity instanceof ContentEntityInterface ? $entity->toUrl() : NULL,
   +      'local_revision_id' => $entity instanceof ContentEntityInterface ? $entity->getRevisionId() : NULL,
   

   Hum as previous code did not handled it. That's ok. But maybe we can add some checks for security in the case of a paragraph entity like on #3162950: Entity import status report broken. And maybe checking if the entity is revisionable for the "->getRevisionId()".

   As previous code did not do that. I will not block the patch for that.

Going to test.

Test ok.

Patch from comment 17 merged.

@ivan.vujovic, can you please take my review points into account in your next patch please?

Patch merged! Thanks :)

I forgot to update the status back to "Needs work".

Thanks @ivan.vujovic for the patch.

Really nice work!

Here is my review.

1. +++ b/modules/entity_share_async/src/Plugin/QueueWorker/EntityShareAsyncWorker.php
   @@ -90,14 +89,10 @@ class EntityShareAsyncWorker extends QueueWorkerBase implements ContainerFactory
   +    $ids = $this->importService->importFromUrl($prepared_url);
   

   From the comment just above, now that importEntities has a $is_batched parameter, is it possible to use it directly?

2. +++ b/modules/entity_share_client/src/Service/ImportService.php
   @@ -128,24 +128,29 @@ class ImportService implements ImportServiceInterface {
   +  public function importChannel(ImportContext $context, bool $is_batched = TRUE) {
   

   I am sorry. I think I was wrong about the usefullness of having the importChannel not batched. Or maybe you see a case where it can be usefull?

   As it may be used for a great amount of entities. Without batch, it is timeout or memory_limit errors ensured.

   Also, not sure that if not batched, only the first page imported will be appreciated.

   If you don't see any usage without batch of importChannel, please revert the changes in this method.

3. +++ b/modules/entity_share_client/src/Service/ImportService.php
   @@ -234,6 +244,19 @@ class ImportService implements ImportServiceInterface {
   +  public function importFromUrl(string $url) {
   

   Nice!

   I think the importChannelPage method can be removed. This one seems better.

4. +++ b/modules/entity_share_client/tests/src/Functional/EntityShareClientFunctionalTestBase.php
   @@ -676,12 +676,11 @@ abstract class EntityShareClientFunctionalTestBase extends BrowserTestBase {
   +    // Imports data from the remote URL.
   

   Nice to have adapted code in test classes!

Thanks @ivan.vujovic for the updated patch!

One last comment. I can make the change myself, only need your opinion.

+++ b/modules/entity_share_client/src/Service/ImportService.php
@@ -119,27 +119,32 @@ class ImportService implements ImportServiceInterface {
+      return [];

+++ b/modules/entity_share_client/src/Service/ImportServiceInterface.php
@@ -20,8 +20,13 @@ interface ImportServiceInterface {
+   * @return array|null
+   *   The list of entity IDs imported keyed by UUIDs, or NULL if batched.

I think it is "void" and not "NULL".

And you are right about the limit. 50 is the default max of JSON:API. I know that there is a module or patch to be able to control it. But we will see when someone will create an issue.

Thanks! This is merged now.