🇵🇰Pakistan @jibran

+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.

The MR looks good let's add some tests.

mcdruid → credited jibran → .

Hehe, tbh, this was added before PHPStan started supporting it. :D

Committed and pushed to 2.2.x

The field UI is being overall in Drupal 10.2 and beyond and this change is to accommodate field UI changes happening in core. Given that it is a won't fix.

Closing it CWAD after #6.

Re-roll #4.

RE #5: the core issue also didn't not add tests for ::getReferenceableBundles() in 🐛 ResourceTypeRepository wrongly assumes that all entity reference fields have the setting "target_type" Fixed . All the test added in that issue is for deprecation. Also unit test is going to only check that for a given mock object we have some array structure that is already being tested in DER Kernel tests so what exactly trying to test then? I'd say we should add functional tests that can help close ✨ Dynamic Entity Reference normalization support Active .

HEAD is failing on 11.x atm after field UI changes. I'm working on fixing it. So ignore the test fails for now but it needs functional test and mentioned above.

This is a duplicate of 🐛 Implement EntityReferenceItemInterface Needs work .

PPMNMI as per the last comment.

1. +++ b/config/install/linkyreplacer.settings.yml
   @@ -2,3 +2,4 @@ internal_patterns: ""
   +exclude_patter: ''
   

   We need a post update hook to update the existing config.

2. +++ b/src/LinkyEntityUtility.php
   @@ -167,6 +167,14 @@ class LinkyEntityUtility implements LinkyEntityUtilityInterface {
   +    if ($exclude = $this->configFactory->get('linkyreplacer.settings')->get('exclude_pattern')) {
   

   We need some kind of validation for valid regex.

3. +++ b/src/LinkyEntityUtility.php
   @@ -167,6 +167,14 @@ class LinkyEntityUtility implements LinkyEntityUtilityInterface {
   +      if (preg_match($exclude, $href)) {
   

   We need tests for this.

@larowlan any feedback on this one?

LGTM as well. Thanks for the MR.

Could it relate to testbot running in a subdirectory?

Must be any idea how to fix this.

Can you please add

Add indexing to entity_id field.

as a new task?

Linked the wrong issue.

HEAD started failing earlier today maybe because of ✨ Add optional validation constraint support to ConfigFormBase Fixed . I think we need to accommodate that one here as well.

Thank you for creating the issue @larowlan

This is not a correct statement in IS
> data_pipelines_data:pipeline_name:type

As per the patch it should be
data_pipelines_data:pipeline_pluginId:type

Patches are welcome!

Let's get Lee's review on this one.