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

prevent problems for URL parameter that do no exist in Post model #514

Merged
merged 6 commits into from
Jul 31, 2020
Merged

prevent problems for URL parameter that do no exist in Post model #514

merged 6 commits into from
Jul 31, 2020

Conversation

mjauvin
Copy link
Contributor

@mjauvin mjauvin commented Jul 22, 2020

A page with the following URL using the BlogPost component will generate an SQL query error when switching locale since the rainlab_blog_posts DB table does not have a field calld category:

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

@LukeTowers
Copy link
Contributor

What other params would there be to be translated other than slug? Why not just explicitly translate slug and nothing else?

@mjauvin
Copy link
Contributor Author

mjauvin commented Jul 22, 2020

I wanted to leave place for extensibility, if someone adds fields to the blog model, for example

@LukeTowers
Copy link
Contributor

Shouldn't they just extend the event themselves in that case?

@mjauvin
Copy link
Contributor Author

mjauvin commented Jul 22, 2020

would adding a new handler that adds the missing fields work? I have doubts...

@LukeTowers
Copy link
Contributor

I have doubts about the original theoretical use case behind the original implementation, can you provide me a concrete example where someone would be using arbitrary columns from the DB table as URL params that they would also want translated?

@mjauvin
Copy link
Contributor Author

mjauvin commented Jul 23, 2020

I can't. I don't mind changing it to only use the slug parameter as you suggested.

@LukeTowers
Copy link
Contributor

LGTM! @bennothommo I'll leave it to you to merge so I don't screw up the release process.

components/Post.php Outdated Show resolved Hide resolved
@bennothommo bennothommo added Status: Revision Needed Requires changes before it can be accepted Type: Maintenance Minor maintenance to the code base (i.e. minor bug fixes, styling fixes, translation improvements) labels Jul 24, 2020
@bennothommo bennothommo modified the milestones: v1.4.2, v1.4.3 Jul 24, 2020
@mjauvin
Copy link
Contributor Author

mjauvin commented Jul 31, 2020

I submited the requested changes @bennothommo

@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: Revision Needed Requires changes before it can be accepted labels Jul 31, 2020
@bennothommo
Copy link
Contributor

Thanks @mjauvin.

@bennothommo bennothommo merged commit 7e1a9d4 into rainlab:master Jul 31, 2020
@mjauvin mjauvin deleted the blogpost-fix branch July 31, 2020 04:59
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