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

Adds a rule to slug #47

Merged
merged 11 commits into from Jul 11, 2016
Merged

Adds a rule to slug #47

merged 11 commits into from Jul 11, 2016

Conversation

jhuet
Copy link
Contributor

@jhuet jhuet commented Jun 30, 2016

What?

Adds a rule to slug a field with its own value or from another field's.

Checklist

  • Added unit test for added/fixed code
  • Updated the documentation
  • Scrutinizer code coverage is 100%
  • Scrutinizer code quality is as high as possible

Linked issue

None.

@jhuet jhuet changed the title Adds a rule to slug a field with its own value or from another field's Adds a rule to slug Jun 30, 2016
@rick-nu
Copy link
Contributor

rick-nu commented Jun 30, 2016

Hey! Thank you very much for your contribution! I really like this addition, I will give it a closer look soon, but from what I've seen now it's looking really good. 👍

@rick-nu
Copy link
Contributor

rick-nu commented Jun 30, 2016

And I will also look into why Scrutinizer isn't building pull requests anymore :\

@jhuet
Copy link
Contributor Author

jhuet commented Jun 30, 2016

Hi @RickvdStaaij, i actually have a little problem with its usage right now :)

I think most people will slug a field based on another one, say a title for example. Now, if for some reason, i don't provide title to the filter, i end up with an empty string slug because i used defaults('') on slug so it would still be filtered if not provided.

To avoid using defaults('') i found out about $allowNotSet = true property which is a better approach but it will always set slug to whatever value i return, even null.

What i think should be the desired result would be to actually have no slug key in the filtered result at all if its field provider is not being filtered either.

Here's an example for comprehension sake :)

Actual result with $allowNotSet = true:

$filter->value('slug')->slug('name');
$result = $filter->filter([
    'foo' => 'bar',
]);
// $result = array(2) { ["foo"]=> string(3) "bar" ["slug"]=> NULL } 

Now the expected result should be :

$filter->value('slug')->slug('name');
$result = $filter->filter([
    'foo' => 'bar',
]);
// $result = array(2) { ["foo"]=> string(3) "bar" } <- no slug because, no slug provider "name"

Any idea on how to acheive this ? Thanks !

@jhuet
Copy link
Contributor Author

jhuet commented Jul 6, 2016

Hi @RickvdStaaij, any thought on the question in my last comment ? Thanks !

@@ -230,6 +230,16 @@ public function replace($search, $replace)
}

/**
* Results that returns a value slugged
*
* @param type $fieldToSlugFrom
Copy link
Contributor

Choose a reason for hiding this comment

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

@jhuet

Should this be string instead of type?

@rick-nu
Copy link
Contributor

rick-nu commented Jul 7, 2016

@jhuet Sorry, I haven't had the time yet to check this out, but it's on the top of my todo list now, so I hope to get back to you soon. One thing that comes to mind, have you checkend https://github.com/particle-php/Filter/blob/master/src/Chain.php#L42? If you state that a rule is not empty, it shouldn't be ignored and turn up with the given value.

@rick-nu rick-nu self-assigned this Jul 8, 2016
@rick-nu rick-nu removed their assignment Jul 8, 2016
@rick-nu
Copy link
Contributor

rick-nu commented Jul 8, 2016

Sorry it took me so long @jhuet, but I have (finally) looked into it in https://github.com/jhuet/Filter/pull/1. I introduced an isEmpty property on the Rule, which can be set to true. This way the value will be ignored when put out of the filter.

I hope this is what you were looking for. If not let me know :)

Rick van der Staaij and others added 2 commits July 8, 2016 16:30
@jhuet
Copy link
Contributor Author

jhuet commented Jul 9, 2016

No problem @RickvdStaaij, i'm glad to add new functionnalities to this library and see you're still around to help if needed :) Thanks !

]);

$this->assertEquals(['test' => 'test'], $result);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@jhuet

How about introducing an explaining variable here to improve clarity?

$data = [            
    'test' => 'test',
];

$this->assertSame($data, $this->filter->filter($data));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, you got it, i'm in a good mood today :)

@rick-nu
Copy link
Contributor

rick-nu commented Jul 11, 2016

Very nice @jhuet, thanks a lot! I'll merge and release a new version :) 👍

@rick-nu rick-nu merged commit 6a66c07 into particle-php:master Jul 11, 2016
@rick-nu
Copy link
Contributor

rick-nu commented Jul 11, 2016

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

Successfully merging this pull request may close these issues.

None yet

3 participants