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

add flexibility to Post setUrl method #506

Merged
merged 7 commits into from
Jul 11, 2020
Merged

add flexibility to Post setUrl method #506

merged 7 commits into from
Jul 11, 2020

Conversation

mjauvin
Copy link
Contributor

@mjauvin mjauvin commented Jun 20, 2020

The blogPosts component calls each posts setUrl() method but cannot provide the currently selected category.

this PR adds a $params array to allow overriding parameters when setting the post's URL.

the PR modifies the blogPosts component by passing the current category when setting each post's url.

@LukeTowers
Copy link
Contributor

Could you provide an example of behavior that this PR now enables? It doesn't quite make sense to me.

@mjauvin
Copy link
Contributor Author

mjauvin commented Jun 20, 2020

If you have blog posts that have more than one category, the post url will be built using the first category instead of the currently selected category. So if your blogPost page has the following url, the category was always the first category:

url = "/post/:slug/:category?"

Now it property assigns the selected category.

components/Posts.php Outdated Show resolved Hide resolved
@bennothommo bennothommo added Status: Testing Needed Requires testing before it can be accepted Type: Maintenance Minor maintenance to the code base (i.e. minor bug fixes, styling fixes, translation improvements) labels Jun 21, 2020
@bennothommo bennothommo added this to the v1.4.2 milestone Jun 21, 2020
@bennothommo
Copy link
Contributor

@mjauvin Looks fine to me - I'll give it a test tomorrow and merge it if it's all good.

@mjauvin
Copy link
Contributor Author

mjauvin commented Jun 28, 2020

Hi @bennothommo, did you get a chance to test it?

@bennothommo
Copy link
Contributor

@mjauvin Sorry, I haven't yet. I will try to get onto it shortly.

@mjauvin
Copy link
Contributor Author

mjauvin commented Jul 6, 2020

Any luck @bennothommo?

models/Post.php Show resolved Hide resolved
models/Post.php Outdated Show resolved Hide resolved
@bennothommo bennothommo added Status: Response Needed Requires / is waiting on a response from at least one of the participants and removed Status: Testing Needed Requires testing before it can be accepted labels Jul 7, 2020
@bennothommo
Copy link
Contributor

@mjauvin I've tested it and it works, but I'm now in two minds about allowing people to override all the parameters - the category, sure, but not so much the ID and slug. What are your thoughts on just allowing the category to be overridden through the third parameter in setUrl?

@LukeTowers
Copy link
Contributor

@bennothommo what are your concerns with the current approach?

@bennothommo
Copy link
Contributor

@LukeTowers mainly just being able to override the other params - such as id and slug. I feel they should be locked down in some way so someone doesn't inadvertently make incorrect URLs.

@LukeTowers
Copy link
Contributor

@bennothommo I feel like if they do, they're doing it on purpose. It's not like they can accidentally pass that data in, they have to be very explicit about it. I feel like it adds the greatest degree of flexibility and support for future params we may want to use without being a massive pitfall for users / developers.

@mjauvin
Copy link
Contributor Author

mjauvin commented Jul 10, 2020

Frankly, I don't personally think this is an issue but I do think adding this is supposed to help flexibility/extensibility so it IS possible to cause mayhem (as it is with many event handlers if we want to go that far)

@bennothommo
Copy link
Contributor

I can't think of a specific scenario where this could cause mayhem without it being the result of someone doing something intentionally stupid, so I'll merge it. There's just one change I'll suggest first before doing so.

models/Post.php Outdated Show resolved Hide resolved
@bennothommo bennothommo added Status: Revision Needed Requires changes before it can be accepted and removed Status: Response Needed Requires / is waiting on a response from at least one of the participants labels Jul 10, 2020
models/Post.php Outdated Show resolved Hide resolved
@LukeTowers LukeTowers added Status: Review Needed Requires review from a maintainer or trusted community member and removed Status: Revision Needed Requires changes before it can be accepted labels Jul 10, 2020
@LukeTowers
Copy link
Contributor

LGTM, @bennothommo you can merge when you're happy with it.

@bennothommo bennothommo added Status: Completed Fix is completed and merged into the dev branch; remaining open until it has been merged into master and removed Status: Review Needed Requires review from a maintainer or trusted community member labels Jul 11, 2020
@bennothommo bennothommo merged commit 610e41a into rainlab:master Jul 11, 2020
@mjauvin mjauvin deleted the add-seturl-params branch July 11, 2020 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Completed Fix is completed and merged into the dev branch; remaining open until it has been merged into master Type: Maintenance Minor maintenance to the code base (i.e. minor bug fixes, styling fixes, translation improvements)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants