🇨🇦Canada @jibran

Yes, we also need to test per permission and make sure all the operations are supported by newly added permissions.

Thanks for pointing out the issue. I have updated the setting. The default branch is now 8.x-1.x.

P.S.: The default branch setting is moved to gitlab now under https://git.drupalcode.org/project/views_field_view/-/settings/repositor...

We need per operation to permission test.

we can't commit this until we add all the tests.

Left some review comments.

Committed and pushed to 1.x. Thank you all.

Saving credits

The changes look good. Let's address some cosmetic comments, and then we are good to go.

Thanks, merged to 1.0.x.

Can we please create a follow-up to fix the test fails?

I'm also +1 with moving it to contirb.

I'm also +1 with moving it to contirb.

As mentioned in #3311180-2: [meta] Determine how to make Shortcut module more useful and maintainable → , I still believe shortcut module would be better maintained in contrib. Not a lot has happened in shortcut issue queue since I posted that comment.

So, I'm still+1 for removing it from core and moving it to contrib.

I'm not the maintainer of this project so I can't do that in this issue unless we move the issue to the DER queue. I have mentioned the name in the git message. Please let me know if you want me to move the issue to the DER queue or create a new issue and assign the credit there

Thank you all! Committed and pushed to 3.x and 4.x branches of dynamic_entity_reference.

Committed and merged to 1.x

The fix was tested by QA and looks good.

I'd say #3204103: Use virtual index instead of triggers → is the way forward here. I'm planning to work on it in a couple of weeks time.

Yup, I'll Mohit update the issue.

jibran → created an issue.

+1 RTBC

xjm → credited jibran → .

fubarhouse → credited jibran → .

Than you so very much.

Moving to 3.x

Can I have one for 3.x as well, please?

Updated the title and re-categories the issue. Let's update the IS with steps to reproduce.

Can we make the patch green?

Done.

larowlan → credited jibran → .

I talked to him and we are happy to give up the namespace.

jibran → created an issue.

Committed and merged to 4.x

Merged to 4.x.

Thank you, the MR looks good to me.

MR looks good to me.

@dpi and I do have a code for link management system based on linky entity module.

Let me check with dpi if we want to share and maintain on drupal.org

Thanks, MR is merged.

jibran → changed the visibility of the branch 3414923-add-scheduled-and to hidden.

jibran → created an issue.

Do we need to the same fix in 4.x?

It would however, require an update path.

NW for this.

I think we can drop 10.1 support for next release.

Yes, [3414197].

It has been removed from both 3.x and 4.x. Hal is not in core anymore. It can provide the service as optional with tests instead of DER changing the container run time to add the service optional.

Removed drupalci test runs. Will create a follow up to remove drupalci and circleci files.

Back to Lee for fixing the test fails.

I'm closing this in favor of ✨ Add DynamicEntityReferenceItemNormalizer Active in the HAL issue.

I'm closing this in favor of ✨ Add DynamicEntityReferenceItemNormalizer Active in the HAL issue.

jibran → created an issue.

Sorted out triggers on gitlab ci.

Merged the MR to 1.x

Marking it RTBC as per the PR comments from Lee.

RE #15: Thank you.
There is 📌 For 10.2 and above the logic to prevent field hooks relating to selection needs to target a different hook Active but up to you.

NW for figuring out the way to create triggers on gitlabCI.

Moving to 4.x.

The remaining fails are 10.2 PHPUnit so going to merge this as is for now as CS, ES, and Stan are green.

Left some comments.

This is ready for a review.

Committed and pushed to 3.x and 4.x

Committed and pushed #14. Thanks!

Here is the roadmap to fix 10.2 integration.

• 🐛 Implement EntityReferenceItemInterface Needs work
• 📌 For 10.2 and above the logic to prevent field hooks relating to selection needs to target a different hook Active
• 📌 PHPCS fixes form Drupal 10.2 Active

jibran → created an issue.

I'm happy to commit #14 as is and fix the remaining fails in follow-up issue(s).

As mentioned in #3354499-6: DynamicEntityReferenceItemNormalizer broken on D10 → , hal is contrib module now so let's remove it from DER and move it to hal.

There is already a PR for it. https://git.drupalcode.org/project/dynamic_entity_reference/-/merge_requ.... It is not merged as it is not passing yet.

I think we can fix this and create a follow up the field settings of DER.

There are some other fails as well. Please see https://github.com/jibran/dynamic_entity_reference/pull/9. Feel free to make it green. I'm trying to find some time but no luck.

Got rtbc on chat so merging it.

We have the passing and failing MRs now. Can I get an RTBC?

Can you please look into CI/CD pipeline fails?

Thanks, committed and pushed to 1.x

Merged the MR.

Added some tests to fix the logic but still to add failing tests.

I had to bump the core version to 10.1

Because "Error: Call to undefined method Drupal\Core\Utility\Error::logException()" without this phpstan complains.

jibran → created an issue.

I ran out of things to complain.

The MR looks good to me.

This is unrelated to this PR but I think we need a config schema for the pipeline plugin and some tests as well. Do you think we should create a follow up?

How about this for a start? I have added a few observations and questions.

1. +++ b/data_pipelines.install
   @@ -319,3 +319,18 @@ function data_pipelines_update_8015(): void {
   +function data_pipelines_update_8016(): void {
   

   Added the hook as the allowed values are stored in the storage settings.

2. +++ b/modules/data_pipelines_elasticsearch/src/Plugin/DatasetDestination/ElasticSearchDestination.php
   @@ -301,6 +301,28 @@ class ElasticSearchDestination extends DatasetDestinationPluginBase implements C
   +    $index_id = $this->configuration['prefix'] . $dataset->getMachineName();
   

   Because of the machine name I had to pass the whole object. Should I just pass the machine name?

3. +++ b/modules/data_pipelines_elasticsearch/src/Plugin/DatasetDestination/ElasticSearchDestination.php
   @@ -301,6 +301,28 @@ class ElasticSearchDestination extends DatasetDestinationPluginBase implements C
   +        $result = $this->getClient()->count($index_params_base);
   

   The sample result is something like this:

   [
       "count" => 90,
       "_shards" => [
         "total" => 5,
         "successful" => 5,
         "skipped" => 0,
         "failed" => 0,
       ],
     ]
   

4. +++ b/modules/data_pipelines_elasticsearch/src/Plugin/DatasetDestination/ElasticSearchDestination.php
   @@ -301,6 +301,28 @@ class ElasticSearchDestination extends DatasetDestinationPluginBase implements C
   +      $this->logger->error("Failed gto get a current count due to @message", [
   

   Needs to fix a typo.

5. +++ b/src/Destination/DatasetDestinationPluginBase.php
   @@ -82,6 +82,13 @@ abstract class DatasetDestinationPluginBase extends PluginBase implements Datase
   +  public function getCurrentCount(DatasetInterface $dataset): int {
   

   Only override the method for ES and state destinations. We don't need one for NULL but do we need one for file? We have not overridden processCleanup for the file so I thought we don't need to override the count as well.

6. +++ b/src/Destination/DatasetDestinationPluginBase.php
   @@ -82,6 +82,13 @@ abstract class DatasetDestinationPluginBase extends PluginBase implements Datase
   +    return 0;
   

   Should this be -1, NULL, or 0? I think 0 is the correct representation here.

7. +++ b/src/Entity/Dataset.php
   @@ -179,7 +179,7 @@ class Dataset extends ContentEntityBase implements DatasetInterface {
   -        DatasetInterface::INVALID_REMOVE => new TranslatableMarkup('Remove records that are now invalid'),
   +        DatasetInterface::INVALID_REMOVE => new TranslatableMarkup('Remove records that are now invalid and outdated'),
   

   This change makes the intentions of the operation clearer.

8. +++ b/src/Entity/Dataset.php
   @@ -335,6 +338,11 @@ class Dataset extends ContentEntityBase implements DatasetInterface {
   +        if ($count < $original_size[$destination_id] && $this->getInvalidValuesHandling() === self::INVALID_REMOVE) {
   

   Do we need to add the invalid value check? Should we not be doing this in either situation?

9. +++ b/src/Entity/Dataset.php
   @@ -335,6 +338,11 @@ class Dataset extends ContentEntityBase implements DatasetInterface {
   +          $queue->createItem(new ProcessingOperation(ProcessingOperationEnum::PROCESS_CLEANUP, $destination_id, range($count, $original_size[$destination_id])));
   

   Should it be $count+1 or just $count is fine? Needs a test to verify.

Thanks

jibran → created an issue.

I think we can just get the existing count from the destination plugin and if the new count is lower than the existing count then we can remove the outdated one after indexing.

Classifying it as a major bug and re- titling it as this is a regression.

Can we please have a test only patch?

Sorry, here is the proper screenshot

jibran → created an issue.

jibran → created an issue.

Here is the screenshot of the core ER field logo.

Which is kind of similar to the first option.

jibran → created an issue.