weixin_39917046
2020-12-08 20:35 阅读 0

Improve match criteria

Hi Josh,

There are two commits in this pull request.

Commit #9057078 addresses some bugs I found in matching feed record for updating where the match fields were Craft element types.

In your match criteria, you are using

$criteria->elementFieldHandle = [12]
to indicate matching the IDs of Craft elements (entries, categories etc.). However, using that criteria format, I couldn't seem to get that to match any of my existing elements. But changing it to use the
relatedTo
criteria attribute seemed to fix the matching of Craft element types.

Also, in the

FeedMe_ProcessService
, doing some debugging I found that from step 2 onwards, the
$criteria
variable initialised in
processFeed()
seemed to somehow be 'polluted/dirty' with the criteria used on the previous step (i.e. some of the criteria values were values used in the previous step). I thought that it might be something to do with that
$criteria
variable being passed around by reference. So the fix I put in place was to initialize it with an actual clone of the Element Type's original base criteria. You may want to do some testing around this to verify if you get the same issue.

Commit #f2d257c is just a little addition I was hoping to have added in that in the future would support matching elements on additional element criteria attributes that have been programatically added using a plugin hook (which I will suggest in a future pull request). I have found this useful where the matching criteria needs to take into account special cases, such as where two elements have the same name etc. I have had success with programatically adding in some custom criteria to the

matchExtra
option (using a plugin hook) to ensure that the correct element is chosen as a match in special cases.

Let me know your thoughts.

该提问来源于开源项目:craftcms/feed-me

  • 点赞
  • 写回答
  • 关注问题
  • 收藏
  • 复制链接分享

4条回答 默认 最新

  • weixin_39625782 weixin_39625782 2020-12-08 20:35

    Thanks for these PR's, I'm going to test these out. A note on the 'dirty' criteria, its actually done for performance reasons. The criteria is setup before the feed starts, and only the attributes you're matching existing element on get changed each step. The performance improvements are small, but certainly noticeable on larger feeds.

    I'll test the rest of your PR shortly, but all looks good from face value.

    点赞 评论 复制链接分享
  • weixin_39917046 weixin_39917046 2020-12-08 20:35

    No probs.

    Regarding the 'dirty' criteria, what I was finding was the following.

    Say on step 1, passing

    $criteria
    into the
    matchExistingElement
    resulted in having
    $criteria->relatedTo
    equal to
    ['and', [21]]
    .

    Then when step 2 executes in

    processFeed()
    , the line
    $criteria = $this-><em>criteria;</em>
    is executed to reinitialise
    $criteria
    with the base feed criteria. But after coming out of
    matchExistingElement
    on step 2,
    $criteria->relatedTo
    would end up with the merged value of step 1 _and step 2 which is not what is expected (e.g.
    $criteria->relatedTo = ['and', [21, 37]]
    ). This would mean the feed element matching would fail later in the process step.

    Not 100% certain, but the same might have been happening with simple criteria elements too (e.g.

    $criteria->dateOfBirth
    ). Please test. If not then it might be something to do with my
    relatedTo
    criteria adding in
    matchExistingElement
    .

    I'm sure the problem has got something to do with PHP variables by reference etc as

    $criteria
    is getting passed by reference into a few methods in the step processing. In fact, it might even be that
    $this->_criteria
    is getting 'dirty', rather than
    $criteria
    (which is being assigned its value on each step).

    In the end, I found that changing

    $criteria = $this->_criteria;
    to
    $criteria = clone $this->_criteria;
    in
    FeedMe_ProcessService::processFeed()
    fixed the problem. This resulted in the start of each step having
    $criteria
    correctly re-initialised with just the base feed criteria before going into
    matchExistingElement
    .

    Perhaps do some testing and see if you are getting the same results.

    点赞 评论 复制链接分享
  • weixin_39625782 weixin_39625782 2020-12-08 20:35

    I don't believe it'll merge the values for criteria on each step. Take matching against a Category for example, where categories is the field handle for our Category field.

    Step 1: $criteria->categories = [0 => 45781]

    Step 2: $criteria->categories = [0 => 45681]

    The above applies for any non-element field, and the values are overwritten each step. Obviously the above doesn't really match because its an element, which is the reason for your relatedTo modification.

    A quick look shows you're using $criteria->relatedTo = array_merge($criteria->relatedTo, $feedValue); so thats likely why. Any reason for this? Commenting out if (empty($criteria->relatedTo)) { ensures that its 'reset' to [and] each step of the query. Anyway, would need to test this out a bit further.

    I would also be very against using clone $this->_criteria due to performance.

    点赞 评论 复制链接分享
  • weixin_39917046 weixin_39917046 2020-12-08 20:35

    Say I set up my feed so that the unique identifier for existing elements contains three fields where each field is a Craft Entries field type. This means that

    $criteria->relatedTo
    needs to end up containing the IDs of each of the entries in those three fields, determined from the feed data on that step (e.g.
    $criteria->relatedTo['and', [14,98,27]]
    ).

    I may be wrong, as I'm only going from memory, but without the array_merge this will not be the case, and only the ID of the last entry field will end up in the

    $criteria->relatedTo
    parameter.
    点赞 评论 复制链接分享

相关推荐