Sydney, Australia
Account created on 2 March 2011, over 13 years ago
#

Merge Requests

More

Recent comments

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

Committed and merged to 1.x

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

The fix was tested by QA and looks good.

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

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.

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

Yup, I'll Mohit update the issue.

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

+1 RTBC

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

Than you so very much.

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia
πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

Moving to 3.x

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

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

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

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

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

Done.

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

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

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

Committed and merged to 4.x

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

Thank you, the MR looks good to me.

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

MR looks good to me.

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

@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

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

Thanks, MR is merged.

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

jibran β†’ changed the visibility of the branch 3414923-add-scheduled-and to hidden.

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia
πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

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

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

I think we can drop 10.1 support for next release.

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

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.

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

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

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

Back to Lee for fixing the test fails.

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

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

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

Sorted out triggers on gitlab ci.

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia
πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

Merged the MR to 1.x

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

Marking it RTBC as per the PR comments from Lee.

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

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

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

Moving to 4.x.

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

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

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

Left some comments.

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

This is ready for a review.

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

Committed and pushed to 3.x and 4.x

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

Committed and pushed #14. Thanks!

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

jibran β†’ created an issue.

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

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

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

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.

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

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.

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

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

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

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.

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

Got rtbc on chat so merging it.

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

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

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

Can you please look into CI/CD pipeline fails?

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

Thanks, committed and pushed to 1.x

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

Merged the MR.

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

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

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

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.

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

jibran β†’ created an issue.

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

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?

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

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.

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

jibran β†’ created an issue.

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

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.

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

Can we please have a test only patch?

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

Sorry, here is the proper screenshot

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia
πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

jibran β†’ created an issue.

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

Here is the screenshot of the core ER field logo.

Which is kind of similar to the first option.

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

jibran β†’ created an issue.

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

The MR looks good let's add some tests.

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

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

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

Committed and pushed to 2.2.x

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

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.

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

Closing it CWAD after #6.

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

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.

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

This is a duplicate of πŸ› Implement EntityReferenceItemInterface Needs work .

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia
  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.

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

@larowlan any feedback on this one?

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

LGTM as well. Thanks for the MR.

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

Could it relate to testbot running in a subdirectory?

Must be any idea how to fix this.

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

Can you please add

Add indexing to entity_id field.

as a new task?

πŸ‡΅πŸ‡°Pakistan jibran Sydney, Australia

Linked the wrong issue.

Production build 0.69.0 2024