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

More conservative variation on PR #2002 #3706

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
4 participants
@dsnopek
Copy link

commented Sep 27, 2018

See #2002 (comment) where I attempted to describe this :-)

This is branched from @eXistenZNL's original PR. I had to rebase it on the 8.x branch for GitHub to say it was mergeable, which I think is why it gives us both credit for his commit. That commit is all his! Not trying to steal credit or anything.

Anyway, this variation should mean that everyone who Drush was working for in the past should keep working (because they have the 'mysql' extension available). And, for everyone that was getting broken (ie. those without the 'mysql' extension) it should start working! So, hopefully this will make the change acceptable.

I tried to run the test suite with Drupal 6 and PHP 7, but it pulls in Drupal 6.38, which is not compatible with PHP 7. The next release of the D6LTS fork of Drupal 6 (probably 6.45) will be compatible! But I don't know how to get the Drush test suite to use it.

But this does allow drush to work with Drupal 6 on PHP 7 in my limited manual testing!

@weitzman

This comment has been minimized.

Copy link
Member

commented Sep 27, 2018

Looks reasonable to me.

@greg-1-anderson

This comment has been minimized.

Copy link
Member

commented Sep 27, 2018

Thanks, @dsnopek. Could you please add the failing test with Drush 6.38, and let me know what is the best way to download and update Drush 6 lts (via PM, via git, etc.), and I'll fix up the test so that it uses the right version and passes.

@dsnopek

This comment has been minimized.

Copy link
Author

commented Sep 27, 2018

So, I don't know for sure that all the test failures are because it's using Drupal 6.38, but I do know that 6.38 will fail bad with PHP 7.2, so it must be responsible for some. :-)

When the tests are running, I can see it say:

Testing on Drupal version   :  6.38

This will get a Drupal 6 core that's compatible with PHP 7.2:

git clone git@github.com:d6lts/drupal.git --branch 6.x
@greg-1-anderson

This comment has been minimized.

Copy link
Member

commented Sep 27, 2018

@dsnopek As an aside, is there an upgrade method for Drupal 6 lts? Just pull HEAD of 6.x? Monitor the tags and check out any new one that appears?

Seems like someone should have made a "check out next semver git tag" tool by now.

@dsnopek

This comment has been minimized.

Copy link
Author

commented Sep 27, 2018

@greg-1-anderson If you have a Drupal 6 site with the mydropwizard module enabled, then Drupal will use the update status data that we publish, so drush up will actually work! But that needs a working Drupal 6 site already

@greg-1-anderson

This comment has been minimized.

Copy link
Member

commented Sep 27, 2018

That's good to know. For testing, I think I'll just pull HEAD of 6.x, or maybe just pull a hardcoded tag like 6.44.

@dsnopek

This comment has been minimized.

Copy link
Author

commented Sep 27, 2018

Well, 6.44 won't work with PHP 7 - those changes are only in HEAD, currently. A release is coming soon :-)

Thanks so much for working on this!

@greg-1-anderson

This comment has been minimized.

Copy link
Member

commented Sep 27, 2018

OK, HEAD it is!

@dsnopek

This comment has been minimized.

Copy link
Author

commented Oct 8, 2018

I just made a D6LTS Drupal 6.45 release which supports PHP 7.2:

https://github.com/d6lts/drupal/releases/tag/6.45

Is there anything we can do to move this PR forward?

@greg-1-anderson

This comment has been minimized.

Copy link
Member

commented Oct 8, 2018

Thanks for making the stable release. I haven't had a chance to get to this yet; I'll try to fit it in. It just needs tests. It appears that the current tests have a comment that claim to be testing Drupal 6, but they actually just tests the same variants of Drupal 7 as the tests above.

@greg-1-anderson

This comment has been minimized.

Copy link
Member

commented Oct 9, 2018

@dsnopek: I tried running the tests just on a clone of --branch 6.x, so that it would always get the latest, and that registered as Drupal 6.44, not 6.45. Should I just clone 6.45 for now?

@greg-1-anderson

This comment has been minimized.

Copy link
Member

commented Oct 9, 2018

@greg-1-anderson

This comment has been minimized.

Copy link
Member

commented Oct 9, 2018

Hm I tried to check out the tag 8.45, but it still reports Drupal 8.44. Don't have time to investigate why right now.

@dsnopek

This comment has been minimized.

Copy link
Author

commented Oct 9, 2018

Ah, crap... I think I messed up the release process! I'll see what I can do to correct that.

@dsnopek

This comment has been minimized.

Copy link
Author

commented Oct 9, 2018

Ok. I rewrote history a little bit, but the release was only just made so I don't think anyone was depending on it yet. Pulling the 6.45 tag should now know that it's 6.45 :-)

@greg-1-anderson

This comment has been minimized.

Copy link
Member

commented Oct 9, 2018

The tests now claim to be running on Drupal 6.45, but when they get to site-install I get the same symptom as before:

PHP Fatal error:  Uncaught Error: Call to undefined function db_result() in /home/travis/build/drush-ops/drush/commands/core/drupal/site_install_6.inc:145
@dsnopek

This comment has been minimized.

Copy link
Author

commented Oct 9, 2018

What are the arguments being passed to drush site-install? The --db-url needs to use 'mysqli://' rather than 'mysql://'. I just tested locally doing drush site-install manually and it seemed to work

@greg-1-anderson

This comment has been minimized.

Copy link
Member

commented Oct 9, 2018

OK that makes sense. I haven't had time to chase down the cause, but it sounds like what you said is it. I'm guessing that the unish test harness needs to be updated in addition to the changes in the code.

@eXistenZNL

This comment has been minimized.

Copy link

commented Oct 9, 2018

@dsnopek thanks for giving credit and thanks for the work you put into it! Let's hope that in the end this gets resolved once and for all 👍

@greg-1-anderson

This comment has been minimized.

Copy link
Member

commented Nov 21, 2018

Merged in future pull requests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.