Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Product ID inserted into page does not preserve original Magento1 logic for replacing illegal characters #13

Closed
danemacmillan opened this issue Nov 2, 2017 · 1 comment

Comments

@danemacmillan
Copy link

danemacmillan commented Nov 2, 2017

We are currently in the process of migrating to Magento2. We use BazaarVoice (BV)'s Connector module in Magento1. Once the Magento2 version is installed, though, it does not pull in any of our customer reviews, ratings, and questions.

The reason this is happening is because the Magento2 module does not preserve the same logic for replacing illegal characters in product IDs from Magento1. I stepped through the code and compared both of the modules.

Magento1:

screen shot 2017-11-02 at 1 46 13 pm

screen shot 2017-11-02 at 1 47 34 pm

Magento2:

screen shot 2017-11-02 at 1 48 31 pm

screen shot 2017-11-02 at 1 48 44 pm

The differences:

There are two important differences to note, and they are the reason why no BV information displays on our under-development Magento2 store.

  • The first screenshot in each group of screenshots displays a section where there is a place cordoned off, via comments, where the original author suggests that the section is a good place for customizations. In Magento1, there is a blank newline between both; there is no special customization. In Magento2, there is. In Magento2, it adds nearly identical logic from the replaceIllegalCharacters method.

  • The replaceIllegalCharacters method in Magento1 uses /[^\w\d\*-\._]/s as a regular expression. The method by that name in Magento2 modifies it by removing the period: /[^\w\d\*-\-_]/s. Magento2 removes the period as a valid character that should not be replaced, which means unlike in Magento1, Magento2 incorrectly replaces the period.

The problem:

These two differences are the reason no data gets pulled into our Magento2 store.

This should be fixed to remain true to the logic and intent from Magento1. Without the fix, this means configurable product data that pulls in data for Magento1, under the allowed characters that BV uses as a format, for example, will not work when the same exact ID is used for Magento2. That is a lot of data to lose. An example between the differences in the product IDs between Magento1 and Magento2 look like this:

  • conf.CM12345
  • conf_bv46_CM12345

I have a pull request incoming that fixes this issue. I will also reply to the email from BV support, which was taking too long to move forward on this issue.

danemacmillan added a commit to linusshops/magento2-extension that referenced this issue Nov 2, 2017
…tween the Magento1 and Magento2 module. This ensures that data will continue to get pulled into Magento2 stores when the product ID has a perfectly legal period in it.
@bangerkuwranger
Copy link
Contributor

I suspect you'll be solving one problem and discovering another here... we did a migration to Magento 2 earlier this year, and there actually appears to be a change in the parser on the BV servers that ingest the feeds that doesn't recognize the period character in category external ids... there is no logical reason for this, but we were told by the internal BV engineers, point blank, that periods are not considered valid and that including the character in external ids will always throw an error. This has proven to be mostly true... it only seems to throw a parsing error when creating NEW categories, but it accepts existing categories with periods in the external id just fine. Additionally, it's not like we can easily revert to using numeric ids (for one thing, they don't always remain the same between separate installs without a db migration... and that's pretty icky in Magento 2, especially in cloud environments), as the magento category structure ends up flattened by the BV parser to the root category when that is used. In your particular case, I strongly suspect that the existing category external IDs with periods in them will be parsed just fine, BUT if you create a new category using the module without this period-to-underscore swap, the entire feed will be invalidated when it tries to create a new external id for a category on the BV side. Fun.
If the BV parse process were a bit more transparent, there might be hope for the parser to revert to its previous behavior, rendering the period a 'legal' character again... but past experience indicates there isn't much chance of that in the near future.
That said... I wouldn't change the module's behavior (since a feed bv can't parse will be less than useful), but to resolve your issue, BV should be able to remap all of the Magento 1 generated ids to the Magento 2 ids, restoring BV content to the client site without any additional work by the client or devs; they did that to support us during the transition, and after the period got 'banned'... not ideal, but at least it resolved the issue.
(Also, not entirely sure why the logic is repeated, either... but it's a non-sequitur if it is never called...)
Can't really fault the devs for BV not informing them of this change... and it's not documented anywhere in BV specs or as part of the XSL either.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants