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

composer require phpactor/phpactor:^0.11 #79

Merged
merged 6 commits into from
Jan 4, 2019
Merged

composer require phpactor/phpactor:^0.11 #79

merged 6 commits into from
Jan 4, 2019

Conversation

zonuexe
Copy link
Member

@zonuexe zonuexe commented Nov 27, 2018

@zonuexe zonuexe requested a review from kermorgant November 27, 2018 06:05
@kermorgant
Copy link
Contributor

Hi @zonuexe
Sorry for being late, I've been quite busy last days. I will have a look at this in the coming days (I haven't used phpactor's develop branch since 0.9.0).

@kermorgant
Copy link
Contributor

Hello,

Just gave it a shot :

  • running 'phpactor-update' took a significant time (makes me think a note in the README about that might be worth it)

  • restarted emacs and got this error in Phpactor Output

In Filesystem.php line 101:
                                                                               
  The "/home/mikael/src/phpactor.el/vendor/phpactor/phpactor/vendor/phpactor/  
  code-builder/templates" directory does not exist ("/home/mikael/src/phpacto  
  r.el/vendor/phpactor/phpactor/vendor/phpactor/code-builder/templates").      

@kermorgant kermorgant added the bug Something isn't working label Dec 11, 2018
Copy link
Contributor

@kermorgant kermorgant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @zonuexe

How did you handle the upgrade on your machine ?

At first sight, something did not work as expected on mine (see previous comment)

@zonuexe
Copy link
Member Author

zonuexe commented Dec 11, 2018

refs #80

#79 (review)

According to the testimony of @ kermorgant, it seems that the
installation was delayed due to the previous change and an error
occurred.
composer.json Outdated
"phpactor/class-to-file": "@dev",
"phpactor/code-builder": "@dev",
"composer/xdebug-handler": "^1.1",
"monolog/monolog": "^1.0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this one was problematic but after removing this line, installation would succeed

Suggested change
"monolog/monolog": "^1.0",

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kermorgant Thanks!

@kermorgant
Copy link
Contributor

Running phpactor-status after upgrading generated an error with this message

phpactor-action-dispatch: Keyword argument :version not one of (:action :parameters)

My guess is that there's been a change in the RPC protocol, but I've asked @dantleech on slack to be sure.

@zonuexe zonuexe changed the title composer require phpactor/phpactor:^0.10 composer require phpactor/phpactor:^0.11 Dec 11, 2018
@kermorgant
Copy link
Contributor

@zonuexe as a sidenote :

I've started targeting develop branch for some pull requests to keep the freedom to experience a bit.

Could the develop branch be used for us as a staging phase for potentially disrupting changes, which I start to think this issue is

(I've been tempted to define the develop branch as the default one but as I was unsure about the scope of that change, I did not. Does that sound ok to make this switch ?)

@zonuexe
Copy link
Member Author

zonuexe commented Dec 11, 2018

@kermorgant
I agree to implement the experimental features in development branch. 👍
We can avoid destroying the environment of users who use MELPA instead of MELPA Stable.

@kermorgant
Copy link
Contributor

@zonuexe thanks !

My guess is that there's been a change in the RPC protocol, but I've asked @dantleech on slack to be sure.

My bad, the issue disapeared after restarting emacs... (phpactor status works)

But still, trying completion ended with this message

{"version":"1.0.0","action":"error","parameters":{"message":"RecursiveDirectoryIterator::__construct(\/home\/mikael\/src\/phpactor.el\/vendor\/phpactor\/phpactor\/vendor\/jetbrains\/phpstorm-stubs): failed to open dir: No such file or directory","details":"0: RecursiveDirectoryIterator::__construct(\/home\/mikael\/src\/phpactor.el\/vendor\/phpactor\/phpactor\/vendor\/jetbrains\/phpstorm-stubs): failed to open dir: No such file or directory\n#0 \/home\/mikael\/src\/phpactor.el\/vendor\/phpactor\/worse-reflection\/lib\/Core\/SourceCodeLocator\/StubSourceLocator.php(87): RecursiveDirectoryIterator->__construct('\/home\/mikael\/sr...', 4096)\n#1 \/home\/mikael\/src\/phpactor.el\/vendor\/phpactor\/worse-reflection\/lib\/Core\/SourceCodeLocator\/StubSourceLocator.php(63): Phpactor\\WorseReflection\\Core\\SourceCodeLocator\\StubSourceLocator->fileIterator()\n#2 \/home\/mikael\/src\/phpactor.el\/vendor\/phpactor\/worse-reflection\/lib\/Core\/So....

jetbrains folder is to be found under vendor, not vendor/phpactor/vendor

@zonuexe
Copy link
Member Author

zonuexe commented Dec 11, 2018

I added jetbrains/phpstorm-stubs.

@kermorgant
Copy link
Contributor

Thx !

Now facing this


In RpcExtension.php line 52:
                                                                               
  Handler "extension_manager.rpc.handler.list" must be provided with a "name"  
   attribute when it is registered                                             
                                                                               

@kermorgant
Copy link
Contributor

Just talked with @dantleech on slack

1- this error might need another tag for the extension-manager dependency
2- we probably should only require phpactor/phpactor which means we should find another way (than adding the dependencies ourself) to solve the path issues I referenced earlier

@dantleech
Copy link

dantleech commented Dec 11, 2018

we probably should only require phpactor/phpactor which means we should find another way (than adding the dependencies ourself) to solve the path issues I referenced earlier

You might want to try just falling back to ../../../ if the vendor directory doesn't exist here

@kermorgant
Copy link
Contributor

@dantleech I tried and it worked. Is that an acceptable solution for a PR ?

@dantleech
Copy link

Yeah sure

@kermorgant
Copy link
Contributor

Hi @zonuexe

You can revert to only requiring phpactor/phpactor (should be fixed whe this PR is merged)

@kermorgant
Copy link
Contributor

@zonuexe you ca now target version 0.11.1 (thanks @dantleech ) which includes the path fix mentioned above.

@kermorgant
Copy link
Contributor

@zonuexe I realized I had access to the branch so I pushed the change. OK for you ?

It is risky to overwrite the platform version, but now seems to work normally.
@zonuexe
Copy link
Member Author

zonuexe commented Jan 4, 2019

@kermorgant Sorry for the late reply.
I noticed that this Phpactor dependency can not be solved with PHP 7.3, but I think that perhaps there is no problem as unit testing is working in my environment.

@kermorgant
Copy link
Contributor

No worry.
I solved that installation issue by doing composer install --no-dev --ignore-platform-reqs.

I understood the issue was related to a development dependency (php-cs-fixer I remember), which would anyway cause composer install --no-dev to fail. Thus, I think the error can be ignored (except the installation needs the ignore-platform-reqs, which I think should be done manually for the time being, no ?)

@zonuexe
Copy link
Member Author

zonuexe commented Jan 4, 2019

@kermorgant
It seems like the problem I saw. I would like to merge this change.

@kermorgant
Copy link
Contributor

kermorgant commented Jan 4, 2019 via email

@zonuexe
Copy link
Member Author

zonuexe commented Jan 4, 2019

@kermorgant Thanks a lot!

@zonuexe zonuexe merged commit f21cfcf into master Jan 4, 2019
@zonuexe zonuexe deleted the phpactor-0.10 branch January 4, 2019 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants