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

isLinkCurrent compatible with PHP 7 typehints #126

Closed

Conversation

@ondrejmirtes
Copy link
Contributor

ondrejmirtes commented Mar 28, 2016

No description provided.

@ondrejmirtes ondrejmirtes force-pushed the ondrejmirtes:islinkcurrent-tests-php7 branch 3 times, most recently from e85c82e to 18357c1 Mar 28, 2016
@ondrejmirtes ondrejmirtes force-pushed the ondrejmirtes:islinkcurrent-tests-php7 branch 2 times, most recently from fabb404 to 3e7e7a0 Mar 28, 2016
@ondrejmirtes ondrejmirtes force-pushed the ondrejmirtes:islinkcurrent-tests-php7 branch from 3e7e7a0 to b1f7565 Mar 28, 2016
@ondrejmirtes

This comment has been minimized.

Copy link
Contributor Author

ondrejmirtes commented Mar 28, 2016

@dg This is a first draft aimed at resolving #124. I added a bunch more tests that should have been part of PR #125, but I didn't foresee the need for them.

I wasn't sure what data structure should I use to pass data from parseLinkDestination, value object with getters might be better but I'm not sure about performance impact so I sticked to a plain array.

@ondrejmirtes ondrejmirtes force-pushed the ondrejmirtes:islinkcurrent-tests-php7 branch from b1f7565 to df2f934 Mar 28, 2016
$current = $parsedLinkDestination['current'];
$component = $parsedLinkDestination['callee'];
$signal = $parsedLinkDestination['signal'];
$fragment = $parsedLinkDestination['fragment'];

This comment has been minimized.

Copy link
@JanTvrdik

JanTvrdik Mar 29, 2016

Contributor

You can use extract

This comment has been minimized.

Copy link
@ondrejmirtes

ondrejmirtes Mar 29, 2016

Author Contributor

That's exactly what I didn't want to do.

Ondřej Mirtes

On Tue, Mar 29, 2016 at 2:35 AM -0700, "Jan Tvrdík" notifications@github.com wrote:

In src/Application/UI/Presenter.php:

  •     }
    
  •     try {
    
  •         $presenterClass = $this->presenterFactory->getPresenterClass($presenter);
    
  •     } catch (Application\InvalidPresenterException $e) {
    
  •         throw new InvalidLinkException($e->getMessage(), NULL, $e);
    
  •     }
    
  • $parsedLinkDestination = $this->parseLinkDestination($component, $destination);
    
  • $destination = $parsedLinkDestination['destination'];
    
  • $scheme = $parsedLinkDestination['scheme'];
    
  • $presenter = $parsedLinkDestination['presenter'];
    
  • $presenterClass = $parsedLinkDestination['presenterClass'];
    
  • $action = $parsedLinkDestination['action'];
    
  • $current = $parsedLinkDestination['current'];
    
  • $component = $parsedLinkDestination['callee'];
    
  • $signal = $parsedLinkDestination['signal'];
    
  • $fragment = $parsedLinkDestination['fragment'];
    

You can use extract


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub

This comment has been minimized.

Copy link
@dg

dg Apr 1, 2016

Member

$parsedLinkDestination['current'] is always FALSE

@dg

This comment has been minimized.

Copy link
Member

dg commented Apr 1, 2016

There is something wrong, now test passes even if you delete all methods from TestControl…

@dg

This comment has been minimized.

Copy link
Member

dg commented Apr 1, 2016

tests: Moved TestControl to separate file to be reusable across tests I think it is not good idea, it brings dependency between different tests. I'll skip this commit and merge other tests to master, ok?

@dg dg closed this in 10a5eb9 Apr 2, 2016
dg added a commit that referenced this pull request Apr 2, 2016
@dg

This comment has been minimized.

Copy link
Member

dg commented Apr 2, 2016

@ondrejmirtes I closed it this way 6d12b3c, and thanks for all the tests!

dg added a commit that referenced this pull request Apr 3, 2016
dg added a commit that referenced this pull request Apr 3, 2016
@ondrejmirtes

This comment has been minimized.

Copy link
Contributor Author

ondrejmirtes commented Apr 4, 2016

Cool 👍

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