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

CodeIgniter\Filters\Filters::pathApplies() shouldn't be private #5923

Closed
pjsde opened this issue Apr 25, 2022 · 8 comments
Closed

CodeIgniter\Filters\Filters::pathApplies() shouldn't be private #5923

pjsde opened this issue Apr 25, 2022 · 8 comments

Comments

@pjsde
Copy link
Contributor

pjsde commented Apr 25, 2022

When we need to extend the CodeIgniter\Filters\Filters class we are unable to use the pathApplies() method because it is set to private. I think that it should be defined as protected because it already allows it to be extended.

If you think it should be as I propose, I can do the PR with the change, otherwise can anybody explain me wy it should be private?

@paulbalandan
Copy link
Member

Increasing visibilities of methods also increases the backward compatibility concerns in the future. Unless necessary, we should limit the number of methods in the public API at a minimum.

Is there a specific use case why you need to extend that method? If none, I don't think setting it to protected is warranted.

@pjsde
Copy link
Contributor Author

pjsde commented Apr 25, 2022

I realize that the lower the degree of visibility of a method, the less the compatibility risk, but this method doesn't seem to me to be specific to the class itself and that it doesn't compromise the compatibility of the class at all.

Imagine that you have the following config, in which you want to use placeholders, as in the Router, which seems logical to me since we are talking about URI validation:

Config\Filters::$globals['before'] = [
    'before' => [
        'csrf',
        'session' => ['except' => '{locale}/login']
    ]
];

In this case I need to extend this method to be able to replace {locale} in the $path with the language of the request, otherwise the rule doesn't work because the uri will not have '{locale}' but the language.

How would you do in this case to be able to use placeholders?

@kenjis
Copy link
Member

kenjis commented Apr 27, 2022

Considering the above use case, protected function pathApplies() seems better.

@paulbalandan
Copy link
Member

I'm still not convinced. Why not normalize the config filters' paths in the extending class's constructor? I believe pathApplies() should not be doing extra logic aside from matching the paths with the URI.

public function __construct($config, RequestInterface $request, ResponseInterface $response, ?Modules $modules = null)
{
    parent::__construct($config, $request, $response, $modules);

    $locale = $this->request->getLocale();

    foreach ($this->config->filters as &$filters) {
        if (isset($filters['before']) {
           $paths = (array) $filters['before'];

            foreach ($paths as &$path) {
                $path = str_replace('{locale}', $locale, $path);
            }

            $filters['before'] = $paths;
        }
    }
}

But since we have two different opinions on this matter, let's wait for other votes. @lonnieezell @MGatner @samsonasik ?

@MGatner
Copy link
Member

MGatner commented Apr 27, 2022

I don't think this needs to be extensible. The use case seems to me more of a "feature request" (and actually a pretty decent one) that might question the input to pathApplies() but (and I think this is what Paul was saying) shouldn't change the behavior of what the current method does.

We have a number of places where we need "URI meta-segment transformation" and a few of them are blocking features or creating issues. Maybe we need a higher-level interpolator that is handling this for us, like as a subset of HTTP\URI specific to a project's routes?

@kenjis
Copy link
Member

kenjis commented Apr 28, 2022

Agreed.
This could be handled by Paul's way.

If we change pathApplies()'s behavior, the method will be more complicated.
It is not the way to go.

@MGatner
Copy link
Member

MGatner commented Apr 28, 2022

@pjsde thank you for the proposal and your willingness to implement it. If you have differing opinions you are still free to state them, otherwise I think we have identified a few alternatives that fit better with our goals of keeping classes tighter.

If you would like help implementing any of these ideas feel free to hop on Slack or head over to the forums.

@pjsde
Copy link
Contributor Author

pjsde commented Apr 28, 2022

After reading your comments, I agree that with my solution would add more complexity to the method and change its behavior.

I will implement @paulbalandan solution, and I'm glad that @MGatner opened the issue #5930

Thank you all

@pjsde pjsde closed this as completed Apr 28, 2022
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

No branches or pull requests

4 participants