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

Allow multibyte to be configured (fixes #605) #685

Merged
merged 1 commit into from
Oct 9, 2023
Merged

Allow multibyte to be configured (fixes #605) #685

merged 1 commit into from
Oct 9, 2023

Conversation

wilr
Copy link
Member

@wilr wilr commented Aug 22, 2022

No description provided.

src/Model/BlogObject.php Outdated Show resolved Hide resolved
@GuySartorelli
Copy link
Member

I'm going to close this due to lack of action. If someone would like to pick up this work feel free to create a new PR with the requested change applied.

@wilr
Copy link
Member Author

wilr commented Oct 8, 2023

Hi @GuySartorelli What diud you want to see changed? As per my last comment the requested changes I don't agree with and as @lerni pointed out - the fix does what it says on the tin

@wilr wilr reopened this Oct 8, 2023
@wilr
Copy link
Member Author

wilr commented Oct 8, 2023

Perhaps it would have been better if the trait was a DataExtension so that config is automatically applied to DataObjects it is applied to.

Perhaps it would be that would require a major API change compared to the current proposed fix which retains existing API.

@GuySartorelli
Copy link
Member

Sounded like Steve was leaning towards preferring an extension to update the value, e.g.

$allowMultiByte = true;
$this->extend('updateAllowUrlSegmentMultibyte', $allowMultiByte);
$filter->setAllowMultibyte($allowMultiByte);

Personally I do prefer the config approach, so I'd probably be okay with your existing PR if the config name was a little more specific - allow_multibyte could easily be used as a config name for allow multibyte for just about any purpose entirely unrelated to this specific use case. If you changed it so something more like allow_urlsegment_multibyte I would be okay with it.

It is preferable to declare private static for new config, but you can't override it on the class if you add that to the trait. Perhaps the solution that would satisfy Steve then is to add those private static declarations on the classes included in this module which do use the trait, so it is explicitly declared where we have the control to do so.

@wilr
Copy link
Member Author

wilr commented Oct 9, 2023

@GuySartorelli Done.

Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

Nice, thanks for that. Just a small change to make per below.

src/Model/BlogCategory.php Outdated Show resolved Hide resolved
src/Model/BlogCategory.php Outdated Show resolved Hide resolved
Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks.

@GuySartorelli GuySartorelli changed the base branch from 3 to 4 October 9, 2023 07:48
@GuySartorelli GuySartorelli merged commit 08e2bd3 into silverstripe:4 Oct 9, 2023
@lerni
Copy link
Contributor

lerni commented Nov 10, 2023

Now in BlogController.php rawurlencode is triggered based on the mb-config from URLSegmentFilter but it should respect SilverStripe\Blog\Model\BlogTag & SilverStripe\Blog\Model\BlogCategory: allow_urlsegment_multibyte config value in order to show BlogPosts associated with Tags/Categories that actually have mb-characters like umlauts in it.

@wilr wilr deleted the wilr-patch-1 branch November 20, 2023 21:48
wilr added a commit that referenced this pull request Nov 20, 2023
addition to #685 also, respect mb-config in controller for tags & categories
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.

4 participants