-
Notifications
You must be signed in to change notification settings - Fork 2k
feat(app): Added controller attributes #9745
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
base: 4.7
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks like a really nice feature to have in the framework.
#[Attribute(Attribute::TARGET_CLASS | Attribute::TARGET_METHOD | Attribute::IS_REPEATABLE)] | ||
class Restrict implements RouteAttributeInterface | ||
{ | ||
private const TWO_PART_TLDS = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should extract this list into a separate config file. There are a lot more domains like this (e.g., com.pl
, gov.pl
, and regional ones like poznan.pl
), and that's just from my country.
There's no point in trying to hardcode every possible combination here. Moving it into a config file would allow developers to add or override entries as needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good call. i might take another look at better detecting subdomains. I just realized I forgot to look how the router was currently doing it like I was planning on. This doesn't feel wonderful but it does seem the most accurate way I could come up with at the time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm looking in the right place, then it seems like the Router is doing this even simpler: https://github.com/codeigniter4/CodeIgniter4/blob/develop/system/Router/RouteCollection.php#L1665
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is doing it simpler. And only account for co.* doubles. Not great. I think what needs to happen is that after this is approved and merged we need to create a new method in the url_helper to detect subdomains, and use a config file for more robust double detections.
$reflectionClass = new ReflectionClass($this->controller); | ||
|
||
// Process class-level attributes | ||
foreach ($reflectionClass->getAttributes() as $attribute) { | ||
try { | ||
$instance = $attribute->newInstance(); | ||
|
||
if ($instance instanceof RouteAttributeInterface) { | ||
$this->routeAttributes['class'][] = $instance; | ||
} | ||
} catch (Throwable) { | ||
log_message('error', 'Failed to instantiate attribute: ' . $attribute->getName()); | ||
} | ||
} | ||
|
||
if ($this->method === '' || $this->method === null) { | ||
return; | ||
} | ||
|
||
// Process method-level attributes | ||
if ($reflectionClass->hasMethod($this->method)) { | ||
$reflectionMethod = $reflectionClass->getMethod($this->method); | ||
|
||
foreach ($reflectionMethod->getAttributes() as $attribute) { | ||
try { | ||
$instance = $attribute->newInstance(); | ||
|
||
if ($instance instanceof RouteAttributeInterface) { | ||
$this->routeAttributes['method'][] = $instance; | ||
} | ||
} catch (Throwable) { | ||
// Skip attributes that fail to instantiate | ||
log_message('error', 'Failed to instantiate attribute: ' . $attribute->getName()); | ||
} | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a performance perspective, if the developer knows they will not use attributes, it might be worth adding an option to skip calling this method entirely. Currently, we perform reflection on both the class and the method for every HTTP request, which could be expensive.
Another option would be to cache these results, at least in the production environment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A config option could work for now. I tried to limit the amount of reflection happening to just those requests, instead of something like scanning all routes files, etc. But a config option is cheap and easy. I definitely think there's more we can do toward optimizing performance. I've been thinking on that for a little bit. More to come in the future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The simpler options seem right for a starter.
It would be worth combining together with https://github.com/kenjis/ci4-attribute-routes However, the use cases are unclear to me. I have never used cache or restrictions. |
I hadn't seen that one. That's an interesting use case! Since that's not a run-time use, though, it handles a different use case, so probably doesn't make sense to merge features. But thanks for sharing it! I need to play around with that at some point.
The cache attribute is an idea I've had for a while now to make it really simple to cache whole endpoints without all of the boilerplate. It never made sense on it's own, though. I recently starting thinking more about where CI fits in the marketplace, where it can be simpler, etc. and I think I may have led us a little astray when I pushed the route declaration files in 4.0 as the primary way to route. Kenjis auto-routing improved that they added is a great middle-ground for ease of use and security. This was an attempt to get auto-routing to feature parity with route declaration files. Many of the other features are already handled by file-based auto-routing (like prefixes, namespaces, etc). Once I get to some doc rewrites that are coming up, I want to present auto-routing first, since it is easy to understand that mapping of files, I think. As a bonus - auto-routing should have higher performance, though I need to look at one thing with it that I know if. As for usefulness of the features? I think they're pretty handy. I've occassionally built dev-only pages/tools and use the environment restrictions to do that. Caching can be a huge win. And if we want auto-routing to be the primary way, it really needed a nicer interface with the controller filters. |
I remember being told that it's better to have a backup value. In case of compatibility. |
Description
This adds the ability to use attributes on controller classes or methods. The primary goal of this is to get us as close to feature-parity to auto-routing improved with the more robust route definitions. This adds provides the following 3 attributes that can be used by any controller (not just auto-routing):
This also brings some of these behaviors closer to the location where they should be run, making it more obvious what's being processed where.
Examples:
Checklist: