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

Some safe API changes to run querypath2 and 3 in parallel #110

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mkalkbrenner
Copy link
Contributor

First some background information:
For the development of HVRawConnectorPHP I decided to use querypath 3.x.
Therefor the drupal integration module HealthVault™ Connect adds querypath3 as library to your drupal installation.
But as soon as you install another contrib module that depends on the QueryPath module which still contains querypath2, you end up with fatal errors because functions like qp() could not be redeclared.

Unfortunately it's not an option to upgrade the querypath library within the drupal QueryPath module, because (for whatever reason) modules like Apache Solr Config Generator don't work with querypath3.

But due to the fact that you introduced a \QueryPath namespace in querypath3, it's not a big problem to run querypath2 and 3 in parallel. But you have to avoid the redeclaration of the global functions qp() and htmlqp() and the class QueryPath must not be part of the global namespace.

So I applied these safe API changes to run querypath2 and 3 in parallel:

  • renamed qp() to qp3()
  • renamed htmlqp() to htmlqp3()
  • renamed \QueryPath to \QueryPath\QueryPath

Now I can explicit use querypath3 in my module.

If querypath2 is not present, these changes happen on the fly for backwards compatibility, so that nobody has to change his existing code:

  • declare function qp() as wrapper for qp3()
  • declare function htmlqp() as wrapper for htmlqp3()
  • declare \QueryPath as class alias for \QueryPath\QueryPath

To demonstrate the backwards compatibility I did not touch the tests in my patch.

What do you think about that?

From my point of view, the API change is OK, because querypath 3 is still very new and even the documentation on querypath.org has not been updated yet.

I'm open for a discussion or any other solution to run qp2 and qp3 in parallel.

- renamed qp() to qp3()
- renamed htmlqp() to htmlqp3()
- renamed \QueryPath to \QueryPath\QueryPath

If querypath2 is not present, these changes happen on the fly for backwards compatibility:
- declare function qp() as wrapper for qp3()
- declare function htmlqp() as wrapper for htmlqp3()
- declare \QueryPath as class alias for \QueryPath\QueryPath
@mkalkbrenner
Copy link
Contributor Author

Thanks for your response and starting the discussion.

Introducing qp3() is more a workaround than a good solution and not an API change I really like.

Basically I don't see a problem in running two versions in parallel when you use namespaces. The real problem is that the functions qp() and htmlqp() and the class QueryPath are declared in the global namespace. And the functions are always loaded - not only if they are required.

For classes switching the version is easy:

use QueryPath3\QueryPath

Unfortunately that does not work for functions.

As long as the two functions exist, I don't see how to fix that on the drupal side as well :-(

Let me think about it ...

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

Successfully merging this pull request may close these issues.

3 participants