search

vuestorefront / magento2-vsbridge-indexer

This is official Vue Storefront, native, Magento2 indexer
https://vuestorefront.io
MIT License
69 stars 89 forks source link

Remove elasticsearch requirement from composer #88

Open janmyszkier opened 5 years ago

janmyszkier commented 5 years ago

The usual setup is that we have vueSF on a separate server than Magento. BUT composer.json (in the state how it is now) requires a specific version of elasticsearch to be installed on the MAGENTO server, then, however, we're able to configure another server host in Magento plugin settings (Magento admin)

Magento 2.3 uses ES 6.1 by default (https://devdocs.magento.com/guides/v2.3/config-guide/elasticsearch/es-overview.html) which will cause the installation to fail with:

Problem 1
    - Installation request for divante/magento2-vsbridge-indexer dev-master -> satisfiable by divante/magento2-vsbridge-indexer[dev-master].
    - Conclusion: remove elasticsearch/elasticsearch v6.1.0
    - Conclusion: don't install elasticsearch/elasticsearch v6.1.0
    - divante/magento2-vsbridge-indexer dev-master requires elasticsearch/elasticsearch ~5.1 -> satisfiable by elasticsearch/elasticsearch[v5.1.0, v5.1.1, v5.1.2, v5.1.3, v5.2.0, v5.3.0, v5.3.1, v5.3.2, v5.4.0].
    - Can only install one of: elasticsearch/elasticsearch[v5.1.0, v6.1.0].
    - Can only install one of: elasticsearch/elasticsearch[v5.1.1, v6.1.0].
    - Can only install one of: elasticsearch/elasticsearch[v5.1.2, v6.1.0].
    - Can only install one of: elasticsearch/elasticsearch[v5.1.3, v6.1.0].
    - Can only install one of: elasticsearch/elasticsearch[v5.2.0, v6.1.0].
    - Can only install one of: elasticsearch/elasticsearch[v5.3.0, v6.1.0].
    - Can only install one of: elasticsearch/elasticsearch[v5.3.1, v6.1.0].
    - Can only install one of: elasticsearch/elasticsearch[v5.3.2, v6.1.0].
    - Can only install one of: elasticsearch/elasticsearch[v5.4.0, v6.1.0].
    - Installation request for elasticsearch/elasticsearch (locked at v6.1.0) -> satisfiable by elasticsearch/elasticsearch[v6.1.0].

Solution:

janmyszkier commented 5 years ago

@afirlejczyk is this something you can approve?

afirlejczyk commented 5 years ago

Hi @janmyszkier Not really. Version 5.* is a minimum, we can change that. Also, we change requirements for elasticsearch library

    "elasticsearch/elasticsearch": "~5.1|~6.1"

so installation should work on Magento 2.3.1. Changes are available on develop and master branch.

I can change the requirement to

    "elasticsearch/elasticsearch": ">=5.1"

but we can't guarantee it will work with all feature elastic search library releases.

janmyszkier commented 5 years ago

@afirlejczyk I strongly disagree with this approach. Not only this is wrong in a world of microservices where correct ES may be on another host than magento is, but it is also giving the wrong impression about what's supported by VueStorefront.

removing this composer requirement, then connecting to the ES version you make for VueSF (which you provide in admin panel) and then checking for version (where curl equivalent is curl -s localhost:9200 | jq '.version.number' ) makes MUCH more sense.

Once you ACTUALLY support ES bigger than 5.6.X you can release new magento2-vsbridge-indexer that doesn't throw connection error anymore on config save with unsupported ES version. Plain and simple error message like "You're trying to connect to ElasticSearch version 6.3.1 but only 5.6.X versions are supported" would be much better from the user perspective than composer requirement. You cannot do much about ES installed for magento, but you can still push the data to ES on another host and work fine with VueSF

afirlejczyk commented 5 years ago

As I mention in another issue it is like a temporary/quickest solution. https://github.com/DivanteLtd/magento2-vsbridge-indexer/issues/60 I won't have time to work on the more appropriate solution (at least in the nearest feature).

@pkarw @bpicho what do you think about propose solution?

jissereitsma commented 4 years ago

@janmyszkier Are you talking about the ElasticSearch server or the PHP client for ElasticSearch (https://packagist.org/packages/elasticsearch/elasticsearch)? Because with composer, only the PHP client is installed, while you can install the ElasticSearch server anywhere you want. It makes sense to include the PHP client of ElasticSearch in Magento, because otherwise there is nothing in Magento that connects to ElasticSearch ... :(

Coeur-Digital commented 4 years ago

Same unable to install dev or anything Problem 1

jissereitsma commented 4 years ago

@Coeur-Digital Please note that the error is not the same, because the versions are totally different.

It seems you are trying to install the indexer which has a dependency with ElasticSearch, while you have installed yourself already ElasticSearch. Which version of ElasticSearch Client did you install and why did you install it?

Coeur-Digital commented 4 years ago

Hello I re-install Fresh install ubuntu 18.04 Magento 2.3 java 11 Elasticsearch 7.7.0 Why I install it, I didn't I just follow the dev.magento recommandation and I got know:

$ composer require divante/magento2-vsbridge-indexer
Using version ^1.16 for divante/magento2-vsbridge-indexer
./composer.json has been updated
Loading composer repositories with package information
Updating dependencies (including require-dev)
Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - Conclusion: don't install divante/magento2-vsbridge-indexer 1.16.1
    - Conclusion: remove elasticsearch/elasticsearch v7.7.0
    - Installation request for divante/magento2-vsbridge-indexer ^1.16 -> satisfiable by divante/magento2-vsbridge-indexer[1.16.0, 1.16.1].
    - Conclusion: don't install elasticsearch/elasticsearch v7.7.0
    - divante/magento2-vsbridge-indexer 1.16.0 requires elasticsearch/elasticsearch ~5.1|~6.1 -> satisfiable by elasticsearch/elasticsearch[v5.1.0, v5.1.1, v5.1.2, v5.1.3, v5.2.0, v5.3.0, v5.3.1, v5.3.2, v5.4.0, v5.5.0, v6.1.0, v6.5.0, v6.5.1, v6.7.0, v6.7.1, v6.7.2].
    - Can only install one of: elasticsearch/elasticsearch[v5.1.0, v7.7.0].
    - Can only install one of: elasticsearch/elasticsearch[v5.1.1, v7.7.0].
    - Can only install one of: elasticsearch/elasticsearch[v5.1.2, v7.7.0].
    - Can only install one of: elasticsearch/elasticsearch[v5.1.3, v7.7.0].
    - Can only install one of: elasticsearch/elasticsearch[v5.2.0, v7.7.0].
    - Can only install one of: elasticsearch/elasticsearch[v5.3.0, v7.7.0].
    - Can only install one of: elasticsearch/elasticsearch[v5.3.1, v7.7.0].
    - Can only install one of: elasticsearch/elasticsearch[v5.3.2, v7.7.0].
    - Can only install one of: elasticsearch/elasticsearch[v5.4.0, v7.7.0].
    - Can only install one of: elasticsearch/elasticsearch[v5.5.0, v7.7.0].
    - Can only install one of: elasticsearch/elasticsearch[v6.1.0, v7.7.0].
    - Can only install one of: elasticsearch/elasticsearch[v6.5.0, v7.7.0].
    - Can only install one of: elasticsearch/elasticsearch[v6.5.1, v7.7.0].
    - Can only install one of: elasticsearch/elasticsearch[v6.7.0, v7.7.0].
    - Can only install one of: elasticsearch/elasticsearch[v6.7.1, v7.7.0].
    - Can only install one of: elasticsearch/elasticsearch[v6.7.2, v7.7.0].
    - Installation request for elasticsearch/elasticsearch (locked at v7.7.0) -> satisfiable by elasticsearch/elasticsearch[v7.7.0].
jissereitsma commented 4 years ago

Please note that solving issues like these requests from you that you dive into composer. You are reposting the same error but not diving into details here. The most important question is: Why did you install ElasticSearch 7? Why do you need it? Because if you don't use it, you should remove it. Plus it conflicts with the ElasticSearch version that VSF requires. The VSF module is not causing this issue. Your existing composer configuration is causing issues.

Make sure to troubleshoot this with commands like composer show elasticsearch/elasticsearch and composer depends elasticsearch/elasticsearch. Before trying to install VSF again and again, stop and answer the question: Why is ElasticSearch currently installed? If it is not needed, remove it. And then try to install VSF. If you need it, your specific configuration has a conflict but that everyone needs to hear from you what this conflict actually is. Why does your current Magento configuration (without having VSF installed!) need ElasticSearch?

Coeur-Digital commented 4 years ago

Please note that solving issues like these requests from you that you dive into composer. You are reposting the same error but not diving into details here. The most important question is: Why did you install ElasticSearch 7? Why do you need it? Because if you don't use it, you should remove it. Plus it conflicts with the ElasticSearch version that VSF requires. The VSF module is not causing this issue. Your existing composer configuration is causing issues.

Make sure to troubleshoot this with commands like composer show elasticsearch/elasticsearch and composer depends elasticsearch/elasticsearch. Before trying to install VSF again and again, stop and answer the question: Why is ElasticSearch currently installed? If it is not needed, remove it. And then try to install VSF. If you need it, your specific configuration has a conflict but that everyone needs to hear from you what this conflict actually is. Why does your current Magento configuration (without having VSF installed!) need ElasticSearch?

Fixed work's like a charm I downgrade elastic and add successfully the module Thanks again

janmyszkier commented 4 years ago

@jissereitsma 

The most important question is: Why did you install ElasticSearch 7?

this no longer uses older versions of ES package https://github.com/magento/magento2/blob/2.4-develop/composer.json#L37

even if you look at branch 2.3 composer.lock, which supports multiple older versions: https://github.com/magento/magento2/blob/2.3/composer.lock#L537 it installs 7.x (which is the most recent available)

and if you look at the elasticsearch repo: https://github.com/elastic/elasticsearch/releases it points out to version 7: https://www.elastic.co/guide/en/elasticsearch/reference/7.7/release-notes-7.7.0.html

in those cases, "why should he NOT install it?" is my question.

binding the composer to specific versions of "magento/module-catalog": ">=102.0.0", is enough. using separate dependency version for something magento already defines is overdoing it and will cause issues.

Even with the above comments you can see there were issues with branch 2.3 where people had to downgrade their packages. Why? Documentation doesn't mention, but we are all composer experts, aren't we? :)

jissereitsma commented 4 years ago

Maybe we are understanding each other perfectly, but if not: I merely pointed out that if you are trying to install package A, which has a dependency with package B, while your installation already contains package C that conflicts with package B, it is a fair question to ask why package C is needed in the first place. That's the question that I was pointing out.

You are saying that ES7 (in my phrasing above, package C) is part of the Magento monolithic repository. True. But it's not answering the question what ES7 is used for. Do you need it? This starts with a simple assumption that Magento its monolithic package structure is providing numerous modules that you are actually not needing (with the worst case of bundled third party extensions breaking CD deployments).

Maybe the magento/module-elasticsearch module is one of them as well? Because that Magento module is not doing anything if all search is running via the VSF1 indexers. When you are replacing the entire frontend with VSF and when you don't have purpose for that module, it is better to remove it (using a composer replace). Less modules is less issues to deal with.

I think the issue of @Coeur-Digital was solved by now.

As for the original question you raised, is removing the ElasticSearch dependency of the VSF1 indexer module really improving things? With Magento 2.4, we'll be moving into a Service Oriented Architecture (and we should definitely not look at the GitHubs composer.json file because that's only for development and not the version we are going to use) where maybe the dependency of the VSF1 code with the ES client code is a must, but where the dependency of the Magento code with the ES client code is dropped.

I definitely would recommend using composer to document those dependencies.

That doesn't solve your initial question: How to make it easier for people to resolve a composer conflict like this? Perhaps a simple documentation of the command composer require elasticsearch:5?

janmyszkier commented 4 years ago

@jissereitsma I did mix up client and server in https://github.com/DivanteLtd/magento2-vsbridge-indexer/issues/88#issuecomment-514074691, I agree, my bad. You're suggesting to require from people to know, debug and fix something that can be easily prevented with correct usage of composer.json, github releases/tags and semver by magento module (this repo) authors. Please do not suggest more complicated approaches to be preferred.

The initial issue I have submitted is still not solved.

Specifically: magento module (this repo) should not REQUIRE ES package to be installed in specific version because it makes it incompatible in a wrong way that causes more problems if magento decides to drop or use more recent release. Making magento module releases compatible with specific magento versions is way more intuitive because you avoid people asking why is there a composer problem with this repo by simply providing releases that work with their magento release.

This is how every module provider solves this issue, and is suggested solution by magento itself: https://devdocs.magento.com/guides/v2.3/extension-dev-guide/package/package_module.html#sample-composerjson-file

jissereitsma commented 4 years ago

Let's slow down: I agree that this is not an ideal scenario and I understand that you want to have a situation where newbie developers or maybe even merchants can solve issues like these. But the reality is that removing a requirement will be not be solving things, but it will make things worse.

The current requirement of the VSF indexer module is that either ES 5 is needed or ES 6 is needed. What you are saying is that it should be compatible with ES 7 as well and you can make this happen by removing the requirement. In my opinion, removing requirements from composer files to work around requirement conflicts is not a good practice. All standards that Magento has introduced toward extension developers actually will tell you that this is not a good practice: It is not playing ball with semantic versioning, it is ignoring the fact that newer major versions of ES might require a code change in your own client usage, it is ground for the Magento Marketplace rules to deny you from sharing your code on Marketplace.

You are referring to a sample code fragment provided by Magento itself. Please be aware that a composer.json like this will actually give warnings with the Marketplace validation rules provided by the same company? Not all that is documented by Magento as a sample should be labeled right away as a best practice. I could also refer to the Product class of Magento and say it is a good example of modelling, but I would be terrible liar.

Back to my point: I'm currently running a Magento shop with the Magento ElasticSearch module successfully removed, because I'm not connecting that shop to ElasticSearch and because ElasticSearch is not a solid requirement for running Magento itself. If I would require the VSF Indexer module on this shop and if your suggestion would be followed, the result would be a broken installation, because composer (a tool used for dependency management) is not installing the required dependencies.

I need to rest my point: Removing the ElasticSearch requirement from the composer.json might solve the other issue that you were reporting (a dependency conflict). However, it introduces yet other issues. If the real issue is the dependency conflict, it would be best that we clean up this discussion and open up another issue labeled "How to make sure that dependency conflicts like these are easily solved?". But if the proposed solution is to remove the exact solution that composer is offering - dependency management - I would say this is not the right solution to any problem.

janmyszkier commented 4 years ago

but it will make things worse. facts please. right now this is a theory with nothing backing it up.

In my opinion, removing requirements from composer files to work around requirement conflicts is not a good practice. Read again, please. I have suggested to replace bad requirement with a more stable one.

"How to make sure that dependency conflicts like these are easily solved?" Simple, by setting MAGENTO version as a dependency. (and it already contains elasticsearch package dependency). If you want to suppport other magento versions, you can still release vsbridge-indexer 1.x and 2.x (as it is currently done) which only supports specific magento versions BECAUSE magento versions already support specific ES versions.

But if the proposed solution is to remove the exact solution that composer is offering - dependency management - I would say this is not the right solution to any problem. It is not, I'm asking to replace ES package requirement with magento package requirement.

However, it introduces yet other issues. Examples please, I trust facts, so let's discuss based on those.