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

Add support for zombie options#284

Open
berliner wants to merge 3 commits into
Behat:masterfrom
berliner:berliner/zombie-options
Open

Add support for zombie options#284
berliner wants to merge 3 commits into
Behat:masterfrom
berliner:berliner/zombie-options

Conversation

@berliner

Copy link
Copy Markdown

#283 Add support for zombie options

@berliner

Copy link
Copy Markdown
Author

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

@aik099

aik099 commented Jun 12, 2017

Copy link
Copy Markdown

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
Copy Markdown
Author

@aik099 Thanks for the clarification.

@berliner

Copy link
Copy Markdown
Author

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

->scalarNode('node_modules_path')->defaultValue('')->end()
->arrayNode('options')
->prototype('scalar')->end()
->defaultValue(array())

Copy link
Copy Markdown
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

->scalarNode('threshold')->defaultValue(2000000)->end()
->scalarNode('node_modules_path')->defaultValue('')->end()
->arrayNode('options')
->prototype('scalar')->end()

Copy link
Copy Markdown
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

->scalarNode('server_path')->defaultNull()->end()
->scalarNode('threshold')->defaultValue(2000000)->end()
->scalarNode('node_modules_path')->defaultValue('')->end()
->arrayNode('options')

Copy link
Copy Markdown
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
Copy Markdown
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

@aik099 aik099 Jul 18, 2017

Copy link
Copy Markdown

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
Copy Markdown

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.

@TerjeBr TerjeBr Aug 15, 2018

Copy link
Copy Markdown

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
Copy Markdown

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

aik099 commented Jun 24, 2017

Copy link
Copy Markdown

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
Copy Markdown
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

aik099 commented Jun 26, 2017

Copy link
Copy Markdown

If there are none, then none needed.

@aik099

aik099 commented Jul 18, 2017

Copy link
Copy Markdown

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

TerjeBr commented Aug 15, 2018

Copy link
Copy Markdown

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

aik099 commented Aug 15, 2018

Copy link
Copy Markdown

@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
Copy Markdown
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

aik099 commented Aug 15, 2018

Copy link
Copy Markdown

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

TerjeBr commented Aug 17, 2018

Copy link
Copy Markdown

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

@TerjeBr

TerjeBr commented Aug 24, 2018

Copy link
Copy Markdown

@everzet Could you or someone else please merge this?

@TerjeBr

TerjeBr commented Sep 13, 2018

Copy link
Copy Markdown

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

No one seems to have time for this PR.

@robbieaverill

Copy link
Copy Markdown

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

@TerjeBr

TerjeBr commented Sep 13, 2018

Copy link
Copy Markdown

Any maintainer of this package seeing this?

Is @stof the only one who can merge things?

@TerjeBr

TerjeBr commented Sep 17, 2018

Copy link
Copy Markdown

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
Copy Markdown

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

@TerjeBr

TerjeBr commented Oct 8, 2018

Copy link
Copy Markdown

@everzet, @stof, @Shashikant86, @robocoder

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

@TerjeBr

TerjeBr commented Nov 14, 2018

Copy link
Copy Markdown

Ping. Could we have some progress on this please?

@TerjeBr

TerjeBr commented Dec 4, 2018

Copy link
Copy Markdown

Hello? Has this project been abandoned or something?

@spolischook

Copy link
Copy Markdown

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

@surelygroup

Copy link
Copy Markdown

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

@surelygroup

Copy link
Copy Markdown

@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.

7 participants