Skip to content
This repository has been archived by the owner on Mar 20, 2024. It is now read-only.

Add support for zombie options #284

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

berliner
Copy link

#283 Add support for zombie options

@berliner
Copy link
Author

This fails for PHP 5.3. I'm not sure what to do with this.

@aik099
Copy link

aik099 commented Jun 12, 2017

This fails for PHP 5.3. I'm not sure what to do with this.

Nothing. The fixing commit is Behat/Behat@3f5c206 , but since MinkExtension tests are using stable version of Behat and not master branch it's failing here.

@berliner
Copy link
Author

@aik099 Thanks for the clarification.

@berliner
Copy link
Author

@aik099 Anything else I can do to support this change?

@@ -44,6 +44,10 @@ public function configure(ArrayNodeDefinition $builder)
->scalarNode('server_path')->defaultNull()->end()
->scalarNode('threshold')->defaultValue(2000000)->end()
->scalarNode('node_modules_path')->defaultValue('')->end()
->arrayNode('options')
->prototype('scalar')->end()
->defaultValue(array())
Copy link
Member

Choose a reason for hiding this comment

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

->defaultValue(array()) is useless. This is already the case for a prototyped array node

@@ -44,6 +44,10 @@ public function configure(ArrayNodeDefinition $builder)
->scalarNode('server_path')->defaultNull()->end()
->scalarNode('threshold')->defaultValue(2000000)->end()
->scalarNode('node_modules_path')->defaultValue('')->end()
->arrayNode('options')
->prototype('scalar')->end()
Copy link
Member

Choose a reason for hiding this comment

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

please use 4 space for the indentation

@@ -44,6 +44,10 @@ public function configure(ArrayNodeDefinition $builder)
->scalarNode('server_path')->defaultNull()->end()
->scalarNode('threshold')->defaultValue(2000000)->end()
->scalarNode('node_modules_path')->defaultValue('')->end()
->arrayNode('options')
Copy link
Member

Choose a reason for hiding this comment

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

please add useAttributeAsKey('name'). The value of the argument is not really useful (as we don't support XML config files, and YAML can define maps natively), but the call itself is necessary to ensure that keys are not lost when merging 2 configs together (and Behat relies on merging config between the default profile and other profiles for insrtance). Without this call, merging treats the array as a sequence, and so reindexes everything with numeric keys.

Copy link
Author

Choose a reason for hiding this comment

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

done, I hope I understood what you wanted

Copy link

@aik099 aik099 Jul 18, 2017

Choose a reason for hiding this comment

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

@stof has @berliner changed what you've requested? If so, then this PR can be merged.

Copy link

Choose a reason for hiding this comment

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

@stof has @berliner changed what you've requested? If so, then this PR can be merged.

Copy link

@TerjeBr TerjeBr Aug 15, 2018

Choose a reason for hiding this comment

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

I can see in the code that it has been changed.
->useAttributeAsKey('name') is on line 48

Copy link

Choose a reason for hiding this comment

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

I see. Looks good then.

@aik099
Copy link

aik099 commented Jun 24, 2017

I'm not sure if we have any tests that confirm, that options from .behat.yml are transferred correctly to the driver instance returned. If we have, then should update them.

@berliner
Copy link
Author

@aik099 I couldn't find any. The only thing I could find was https://github.com/Behat/MinkExtension/blob/master/spec/Behat/MinkExtension/ServiceContainer/Driver/ZombieFactorySpec.php which didn't seem very specific. If you can give me a hint on how to add specific tests there I would gladly try to add these.

@aik099
Copy link

aik099 commented Jun 26, 2017

If there are none, then none needed.

@aik099
Copy link

aik099 commented Jul 18, 2017

The minkphp/MinkZombieDriver#183 is merged now and (if this repo is using dev versions of all drivers) we can actually see if it works.

@TerjeBr
Copy link

TerjeBr commented Aug 15, 2018

Hi. Any updates on this PR?

I just wanted to set a zombie option, and found that it was no way to do it.
If this PR is ready to merge, please merge it.

@aik099
Copy link

aik099 commented Aug 15, 2018

@TerjeBr , the current state of PR is @stof and @berliner haven't replied to the only question I've asked about code changes. Only that is blocking the merge.

@berliner
Copy link
Author

@aik099 I was waiting for a confirmation by @stof at the time, which is why I didn't answer to you. I have moved to headless chrome in the meantime, so I can't support this PR any longer.

@aik099
Copy link

aik099 commented Aug 15, 2018

PR is ready for merge. I'm missing merge permissions on this repository unfortunately, so it's the @stof , who can do the merging part here.

@TerjeBr
Copy link

TerjeBr commented Aug 17, 2018

@stof please merge this. I am waiting for it.

@TerjeBr
Copy link

TerjeBr commented Aug 24, 2018

@everzet Could you or someone else please merge this?

@TerjeBr
Copy link

TerjeBr commented Sep 13, 2018

@robbieaverill Could you have a look at may be merging this?

No one seems to have time for this PR.

@robbieaverill
Copy link

@TerjeBr I'm not a maintainer of this package, sorry!

@TerjeBr
Copy link

TerjeBr commented Sep 13, 2018

Any maintainer of this package seeing this?

Is @stof the only one who can merge things?

@TerjeBr
Copy link

TerjeBr commented Sep 17, 2018

What will it take to get this merged? Has this project been abandoned?

Seems like no project mantainer with merge rights is around?

@robbieaverill
Copy link

@TerjeBr with respect, good things take time in open source. If you need the change you can fork the repository :)

@TerjeBr
Copy link

TerjeBr commented Oct 8, 2018

@everzet, @stof, @Shashikant86, @robocoder

Anyone that have merge permissions to this repo give this some attention please.
Thank you.

@TerjeBr
Copy link

TerjeBr commented Nov 14, 2018

Ping. Could we have some progress on this please?

@TerjeBr
Copy link

TerjeBr commented Dec 4, 2018

Hello? Has this project been abandoned or something?

@spolischook
Copy link

I guess too few members used zombie (even I), and so interested in this.

@surelygroup
Copy link

@stof Could you or some one else please merge this PR?

@surelygroup
Copy link

@everzet A friendly ping ...

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants