search

weprovide / valet-plus

Blazing fast macOS PHP development environment
1.55k stars 208 forks source link

Homebrew/php is deprecated in favor of homebrew/core #127

Closed Neodork closed 6 years ago

Neodork commented 6 years ago

What is the problem? Homebrew/php is deprecated in favor of homebrew/core which now has support for php formulae. Homebrew/core supports the following php versions: 5.6, 7.0, 7.1, 7.2. When people use some valet or brew commands which trigger an update they will brick their valet+ installation. Due to PHP not being able to find the missing extensions which are now managed by PECL.

The new homebrew/core php packages have several extensions prepackaged like: mcrypt and intl. Packages should now be managed using the PHP package manager PECL.

What to do?

Pecl manager supported versions

Current progress can be followed at https://github.com/tamtamnl/valet-plus/tree/hotfix/homebrew-core-php and will soon be followed up by a PR for the community to judge.

Edit: Merged.

samgranger commented 6 years ago

Thank you for your work so far. We are also trying to push out a fix asap for this issue - your work so far is really appreciated!

Neodork commented 6 years ago

Brew has changed its stance regarding PHP 7.0 and will now support PHP 7.0. See: https://github.com/Homebrew/homebrew-core/pull/26026#event-1553827401

@samgranger Is there someway we can contribute together? Without making each others work obsolete? Are there any communication channels for contributors?

martisj commented 6 years ago

@Neodork https://gitter.im/ maybe?

Neodork commented 6 years ago

@martisj Yeah if the owners of the repository could set that up, I'm all down with gitter.

samgranger commented 6 years ago

Setting up gitter now.

samgranger commented 6 years ago

https://gitter.im/valet-plus/

rvetrsek commented 6 years ago

We have all bricked our Valet+ installations on our team this morning because of this and our development was halted for a couple of hours. Is there an ETA for a fix?

Tjitse-E commented 6 years ago

@ObSeSSeN did you already fix your local installation? I've switched to the original Laravel/Valet, reinstalled Valet and linked PHP@7.1 with brew link php@7.1 --force --overwrite and after a restart i've got Valet working again with Magento 2.

rvetrsek commented 6 years ago

@Tjitse-E yes but we used MAMP because we need to switch between PHP 5.6 and 7.1 on a regular basis. We will definitely switch back to Valet+ as soon as a fix is released, it is, in my opinion, a faster and more lightweight solution.

Neodork commented 6 years ago

ObSeSSeN My goal is to finish the new package manager within the next 48 hours. Once completed valet+ should be operational again for non bricked php installations. I am currently testing the new Pecl extension manager. The current support is:

rvetrsek commented 6 years ago

@Neodork that is excellent news, I can't wait! Thank you for all your effort!

rvetrsek commented 6 years ago

@Neodork I now already use your latest hotfix version and so far it works great when switching between PHP 5.6 and 7.1.

Neodork commented 6 years ago

@ObSeSSeN Great, nice to have some input on the latest hotfix version. Let us know here if you encounter any problems regarding switching/extension management. @samgranger mentioned there were still some issues regarding PHP 7.2 so I will be looking into that.

navidfalla commented 6 years ago

@ObSeSSeN could you tell me how were you able to use the tamtamnl's repo to install valet-plus? I'm trying to do so, but nothing works. I'm thinking to go through the commits and revise the files of a new installation of weprovide/valet-plus one by one. Did you do it in a more efficient way?

rvetrsek commented 6 years ago

@navidfalla I had Valet+ installed globally by Composer, then I went into the ~/.composer/global folder and replaced the valet-plus folder by cloning a new one from the tamtamnl/valet-plus repo. I then moved into the valet-plus folder and checked out the hotfix/homebrew-core-php branch. The last step was to do a composer install inside the same folder.

Quick and dirty I know but it will do until the PR is merged.

rvetrsek commented 6 years ago

@Neodork I haven't tested this thoroughly but I think there is a problem when switching PHP versions (eg. from 5.6 to 7.1) in that the Nginx still uses the 7.0 version. I did the switch and in the console the php -v command outputs the 7.1.16 version from brew but if I output phpinfo() on my website it says that the version is 7.0.27.

I fixed it by restarting my computer. Somehow somewhere there was a php70-fpm socket still running - even thou I removed all PHP versions before trying the hotfix branch.

jahvi commented 6 years ago

@navidfalla You can change your global composer file to use the hotfix branch for now as a "cleaner" way to try it, your composer.json file should look something like this:

{
    "require": {
        "weprovide/valet-plus": "dev-hotfix/homebrew-core-php"
    },
    "repositories": [
        {
            "type": "vcs",
            "url": "https://github.com/tamtamnl/valet-plus"
        }   
    ]
}

I haven't tested it but it should work.

code2prog commented 6 years ago

did not work :/ 2018-04-05_10-23-38

Neodork commented 6 years ago

@code2prog First you need to remove your weprovide/valet-plus from the composer.json and then do composer global update.

Then add the lines as mentioned by @jahvi however you need to change a small typo:

{
    "require": {
        "dept/valet-plus": "dev-hotfix/homebrew-core-php"
    },
    "repositories": [
        {
            "type": "vcs",
            "url": "https://github.com/tamtamnl/valet-plus"
        }   
    ]
}

And do composer global update again.

Tjitse-E commented 6 years ago

@Neodork i've just tested your fork (branch dev-hotfix/homebrew-core-php) and Valet is working correctly again, including Xdebug. I did had to remove my old PECL folder manually, also a reboot of my mac was needed to get things running again. Many thanks!

Neodork commented 6 years ago

How do I unbrick my PHP installation for Valet+ to work? P.S: We will add this to Valet+ soon, so when someone uses valet use {version} valet install valet fix it will trigger the fix automatically.

  1. Make sure all old PHP homebrew/php packages are removed:

    brew list | grep php56- | xargs brew uninstall
brew list | grep php70- | xargs brew uninstall
brew list | grep php71- | xargs brew uninstall
brew list | grep php72- | xargs brew uninstall
brew list | grep n98-mag | xargs brew uninstall
brew list | grep drush | xargs brew uninstall
brew uninstall php56
brew uninstall php70
brew uninstall php71
brew uninstall php72
brew untap homebrew/php

    P.S: Do not uninstall php71 for now. n98magerun2 has a dependency for it. Added new implementation in merged PR. Binaries are now downloaded by the binary manager.

  2. Install Valet+ default php version:

    brew install php@7.1
brew upgrade php@7.1
brew link php@7.1 --overwrite --force

  3. Restart the terminal.

  4. Disable old extension configuration from homebrew/php:

    
mv /usr/local/etc/php/5.6/conf.d/ext-apcu.ini /usr/local/etc/php/5.6/conf.d/ext-apcu.ini.disabled
mv /usr/local/etc/php/5.6/conf.d/ext-geoip.ini /usr/local/etc/php/5.6/conf.d/ext-geoip.ini.disabled
mv /usr/local/etc/php/5.6/conf.d/ext-intl.ini /usr/local/etc/php/5.6/conf.d/ext-intl.ini.disabled
mv /usr/local/etc/php/5.6/conf.d/ext-mcrypt.ini /usr/local/etc/php/5.6/conf.d/ext-mcrypt.ini.disabled

mv /usr/local/etc/php/7.0/conf.d/ext-apcu.ini /usr/local/etc/php/7.0/conf.d/ext-apcu.ini.disabled mv /usr/local/etc/php/7.0/conf.d/ext-geoip.ini /usr/local/etc/php/7.0/conf.d/ext-geoip.ini.disabled mv /usr/local/etc/php/7.0/conf.d/ext-intl.ini /usr/local/etc/php/7.0/conf.d/ext-intl.ini.disabled mv /usr/local/etc/php/7.0/conf.d/ext-mcrypt.ini /usr/local/etc/php/7.0/conf.d/ext-mcrypt.ini.disabled

mv /usr/local/etc/php/7.1/conf.d/ext-apcu.ini /usr/local/etc/php/7.1/conf.d/ext-apcu.ini.disabled mv /usr/local/etc/php/7.1/conf.d/ext-geoip.ini /usr/local/etc/php/7.1/conf.d/ext-geoip.ini.disabled mv /usr/local/etc/php/7.1/conf.d/ext-intl.ini /usr/local/etc/php/7.1/conf.d/ext-intl.ini.disabled mv /usr/local/etc/php/7.1/conf.d/ext-mcrypt.ini /usr/local/etc/php/7.1/conf.d/ext-mcrypt.ini.disabled

mv /usr/local/etc/php/7.2/conf.d/ext-apcu.ini /usr/local/etc/php/7.2/conf.d/ext-apcu.ini.disabled mv /usr/local/etc/php/7.2/conf.d/ext-geoip.ini /usr/local/etc/php/7.2/conf.d/ext-geoip.ini.disabled mv /usr/local/etc/php/7.2/conf.d/ext-intl.ini /usr/local/etc/php/7.2/conf.d/ext-intl.ini.disabled mv /usr/local/etc/php/7.2/conf.d/ext-mcrypt.ini /usr/local/etc/php/7.2/conf.d/ext-mcrypt.ini.disabled


P.S: To check if you removed all missing extensions you can do `php -v` and see if it throws any warning.

~~5. Add the repository by your prefered method. For example as explained in https://github.com/weprovide/valet-plus/issues/127#issuecomment-378900526. P.S: `dept/valet-plus` is not a supported repository and I would advice to switch back as soon as the PR is merged.~~ MERGED in version 1.0.12.

~~6. When the repository is installed, use Valet+ like you normally would. Start with `valet install`.~~ Use valet like you normally would.

Edit: After merge changes.

@Tjitse-E Thanks for providing feedback good to know it works. I shall continue to create functionality for the above. Yes the PECL extension management is still a bit fragile. I intend to tackle this in another issue/pr when this one is merged.
samgranger commented 6 years ago

Just thinking out loud here, would it be prettier to add to valet install instead of switch/fix?

jahvi commented 6 years ago

@Neodork Thanks I should have tested it before commenting 😅

Any reason why the package name should be dept/valet-plus instead of weprovide/valet-plus?

Neodork commented 6 years ago

@samgranger Agreed, will add check into valet install (PhpFpm#install()) instead. It fits better since users should use valet install after switching versions.

@jahvi That is because our company name is dept (even though github says tamtamnl, we actually renamed to dept the 3rd of april). The default branch of the repository is master-dept, which is a branch with changes only for usage within dept. Because the default branch is master-dept and not master the repository will make up it's composer file from the master-dept branch when linking directly from github as mentioned in https://github.com/weprovide/valet-plus/issues/127#issuecomment-378900526.

We do this so we can merge the upstream of Valet+ to the master branch. We then review the changes made by Valet+. If approved we merge master to dept-master so our colleagues can make use of the updated Valet+. As for branching for new features/hotfixes, unless the feature is specifically for master-dept, we branch off from the master branch which is identical to the Valet+ master branch. Then when we're certain of our implementation we can PR back to weprovide/valet-plus without adding changes that are specific to the needs of dept.

jahvi commented 6 years ago

@Neodork Ah that makes sense, thanks!

markitosgv commented 6 years ago

Hi,

Im trying to install from zero valet+.

First i've installed php 7.1.16 from homebrew/core, then I follow #127 comment.

When I try to install valet+ throws this error: "Could not find installation path for: apc"

php -v output:

PHP 7.1.16 (cli) (built: Mar 31 2018 02:28:54) ( NTS )
Copyright (c) 1997-2018 The PHP Group
Zend Engine v3.1.0, Copyright (c) 1998-2018 Zend Technologies
    with Xdebug v2.6.0, Copyright (c) 2002-2018, by Derick Rethans
    with Zend OPcache v7.1.16, Copyright (c) 1999-2018, by Zend Technologies

php -m

[PHP Modules]
apcu
bcmath
bz2
calendar
Core
ctype
...cutted for readability...
[Zend Modules]
Xdebug
Zend OPcache

imagen

Do you know what's wrong?

P.S: To help someone, you can add "brew tap kyslik/php" to work as before homebrew/php.

davidstillson commented 6 years ago

@Tjitse-E What folder did you have to remove when you got rid of PECL?

Tjitse-E commented 6 years ago

@davidstillson, I think it was /usr/local/share/pear, but I'm not 100% sure. It contained only two empty folders in my case, so it was safe to delete. Maybe you can try renaming it first to see if it works? I had the same error as @markitosgv ('Could not find installation path for ...)

ArjenMiedema commented 6 years ago

Just wanted to say thanks for this fix and your work on it. For now, the fix from @Neodork works for me.

kevyworks commented 6 years ago

I saw this package after I got this homebrew/core thing changed, and I do wish they don't do this kind of thing again, enough said. I would love to see this merge to weprovide/valet-plus soon. So maybe monday could jump start things each of our dev-opts life again. Currently using Laravel's valet, and I was able to document my installation script.

Deejay-Ste commented 6 years ago

Hi guys,

I'm trying everything to make my Valet+ work for 2 days now but it doesn't want to. I tried @Neodork fix but I have the same problem as @markitosgv with another module :

"Could not find installation path for: geoip"

I have the same version of PHP with the same modules, except geoip. So I try to install it through PECL manually using pecl install geoip but I have this error :

ERROR: '/private/tmp/pear/temp/geoip/configure --with-php-config=/usr/local/opt/php@7.1/bin/php-config' failed

Any tips to make it work please ? Thanks for your time.

davidstillson commented 6 years ago

@Deejay-Ste I was able to correct this by manually installing the packages with PECL and then doing another pass at valet install

pecl install geoip I got a nasty error about it failing, but then I tried valet install again and I was golden.

Neodork commented 6 years ago

@ArjenMiedema My pleasure, the community has been very positive! I appreciate all the kind words from everyone that has shown their appreciation for the upcoming PR.

@ other comments, I will look through the comments as soon as I have some more time. Today I couldn't find any. Good news is I have a WIP implementation for custom PECL packages like Ioncube. I am currently working on the install/uninstall. I hope to have the PR ready for review on monday.

Image of Yaktocat

MaxSouza commented 6 years ago

For me, when execute valet install return:

image

Neodork commented 6 years ago

@markitosgv That's an error when PECL couldn't install apcu_bc. Apparently there is a problem when running pecl install apcu_bc. You could run it manually to get more information on the error. I will look into improving the indescriptive error messages.

@Deejay-Ste Same as above and as @davidstillson mentions pecl install geoip is the command to check what goes wrong in your installation.

@MaxSouza @george-kar I've noticed that that n98magerun packages were part of homebrew/php and they will now need to be installed using a different method. I think this is also the case in https://github.com/weprovide/valet-plus/issues/126.

I've finished up the ioncube loader implementation for all PHP versions supported by valet+. I will now look into the fix command and improving the error logging. And then I will have to devise some acceptable way to install custom phar files that are not supported by brew @samgranger any thoughts?

@jahvi Since you mentioned that ioncube installation didn't work in https://github.com/weprovide/valet-plus/issues/121. Could you test the new implementation and see if this works for you?

davidstillson commented 6 years ago

@Neodork Thanks for all the great work on this, btw. You are literally saving my productivity. :-)

MaxSouza commented 6 years ago

@Neodork Thanks for all the great work.

amcastror commented 6 years ago

Hi @Neodork, thanks for all the work.

I've noticed that that n98magerun packages were part of homebrew/php and they will now need to be installed using a different method

This means that we should try and install n98magerun before valet+, and then the install will complete? Do you happen to know how this could be done?

Thanks a lot!

Neodork commented 6 years ago

@davidstillson @MaxSouza @amcastror As always, my pleasure!

@amcastror If you download the latest version from my hotfix (as mentioned in https://github.com/weprovide/valet-plus/issues/127#issuecomment-378900526) the n98-magerun devtools will be removed from the valet install command. You will then be able to do valet install again without getting an error about it.

jahvi commented 6 years ago

@Neodork I can confirm Ioncube is working for me now, thanks!

samgranger commented 6 years ago

Running pecl channel-update pecl.php.net probably solves most errors related to apc.

In some scenarios you need to run brew install geoip before you can run pecl install geoip.

People getting phpize errors, please run brew install autoconf

Neodork commented 6 years ago

@jahvi Thanks for checking that for me, I appreciate your swift response.

Deejay-Ste commented 6 years ago

Thanks @Neodork @davidstillson and @samgranger ! You saved the day !

samgranger commented 6 years ago

As mentioned in gitter, it looks like we cannot run the fix function during valet install due to commands being run under sudo as defined in ./valet

Maybe we can still do the check in valet install & throw an error requesting the user to run valet fix if this is required.

wowcut commented 6 years ago

What is the status on this? Brew upgrade broke all dev machines here!

Regarding the procss: wasn't it possible to view this change coming by watching beta versions of brew? I know "we are all hobbyists etc..." but how could this come in that surprising way??? This is not meant as a rant but I would like to know what could be done in future by less active and more 'consumers' of this project to prevent such a thing ever happening again.

BTW the workaround in this comment https://github.com/weprovide/valet-plus/issues/127#issuecomment-378870073 now requires a github oauth token because of API rate limit.

wowcut commented 6 years ago

The procedure described here does not work, I get on ALL browser requests to any valet site:

Fatal error: Call to undefined function apcu_fetch() in /Users/xyz/.composer/vendor/dept/valet-plus/server.php on line 37

valetplus is still broken.

Neodork commented 6 years ago

@wowcut Officially this is just a fork of laravel/valet so I would've expected laravel/valet to have fix ready. That was not the case. But I had need for this fix myself in the easter weekend (was going to work on some sites) so started myself. There were no existing PR's or assigned issues at the time. I am unsure what the upstream is doing about it but I am keeping an eye on: https://github.com/laravel/valet/pull/561 since that is where they are working on the new version management. And we'll likely need to merge their changes soon.

Valet+ should've reacted to the changes of laravel/valet but since there was no notion of it, the contributors missed that, it happens. We're doing our best to restore functionality to Valet+ as soon as possible. Today @samgranger did some tests with the current hotfix and got back to me with some feedback. Tonight I will be looking into custom binaries like n98-magerun and drush. Once I've finished the custom binaries I can format all code and submit the PR.

Fatal error: Call to undefined function apcu_fetch() in /Users/xyz/.composer/vendor/dept/valet-plus/server.php on line 37 You likely didn't install the new config with valet install. In z-performance.ini APCu is disabled. You need to atleast enable the CLI version and then restart the entire environment preferably with valet restart.

Edit: Part about Valet+

wowcut commented 6 years ago

BTW: why does happen so much in server.php? Wouldn't it be wise to make these install scripts just add another vhost like configured and not intercept each and every request with server.php?

E.g. why is server.php using apcu_fetch() for each and every request?

I really would like to read more about the reasoning behind server.php - this is not the first time everything I wanted to do today is blocked because of some issue with this server.php script and it looks to me like it tries to do too much.

EHLOVader commented 6 years ago

server.php is there because it provides a way to manage the location of the webroot using a driver based system. You can see the history of it on laravel/valet here https://github.com/laravel/valet/commits/master?after=74e203e37b9549b364feda1920e91e3156f38ad9+34&path%5B%5D=server.php

I would say the apc cache was added to improve performance. Here is the commit when it was added https://github.com/weprovide/valet-plus/commit/72900d8cf19f7cf982dd00559b918b3145e68fc3#diff-c7b36e05b81cc4174a915d1bd510c758 .

I have seen in comments that removing it can slow the system down quite a bit.

Following the directions in NeoDork's post above https://github.com/weprovide/valet-plus/issues/127#issuecomment-378914908

And the added advice to install pecl libraries manually https://github.com/weprovide/valet-plus/issues/127#issuecomment-379549204 helped me on Friday, but coming in today I'm getting mysterious "Did not get front controller from driver." errors. So I'm diving back in.

Did you know when we will have a polished Pull request. I see there has been much progress over the weekend.

Thanks for the work on this.

Neodork commented 6 years ago

Just rebased the PR and added binary management for n98-magerun, n98-magerun2 and drush launcher. Will now proceed to format the changes and rebase again with the upstream.