Enforce prefix requirements #138

Closed
wants to merge 1 commit into
from

Projects

None yet

3 participants

@ghost

We already discussed how namespace prefixes should terminate in \, but this is not 100% clear in the spec.

@pmjones

I've played with this a little, and while it is strict and informative, I don't think it's necessary. In the example implementations, we are stripping the leading backslash from the incoming class name and manually adding a backslash separator. For that reason, I think it would be sufficient to trim() the namespace prefix here.

@pmjones

Call it a variation on "be loose in what you accept". ;-)

@simensen

Are we supposed to discuss this on list or here?

I don't really like this for a number of reasons. First of all, it implies that someone might actually ever want to register Dflydev\Canal and have it match the class Dflydev\CanalFoo. As far as I know, nobody is doing that and I'm not sure why you'd even want to?

The general purpose autoloader doesn't require extra shenanigans to get this to work out of hte box. The prefix is supposed to be a namespace prefix, right? So under no circumstances should registering Dflydev\Canal imply that a class in Dflydev namespace that starts with Canal should be matched.

I set his change to the spec trying to account for specific implementation details that are unnecessary. Forcing people to end their namespace with the trailing \ when the specification clearly states that the prefix is supposed to be a namespace is forcing the end user (the person registering the namespace prefix) to do work that the autoloader implementing the spec should be doing.

For example, the general purpose autoloader is immune to this problem anyway. The project-specific autoloader was not so it was fixed to ensure that in that specific implementation we had to ensure that we closed the door for matching classnames. No changes to the spec were required.

@pmjones

Yes, after thinking about this a while, I think it's not really needed.

@pmjones pmjones closed this Jun 6, 2013
@ghost

I love how conversations have to be repeated. The point is how you match the namespace. If you are doing a string search for example, and autoloader might respond to Foo for FooBar. The trailing slash is required, we've already discussed this and agreed upon that fact on the list. Having Foo\ means an autoloader will not respond to FooBar\ when it's been registered for Foo\.

@ghost

@pmjones will you please not act like judge and jury. PRs and discussions need time to mature and for people to find time to think and respond. PRs/discussions need time. It seems like this is your project and we need to convince you. If we do it's accepted, if not, it's not. That's kind of what I've been trying to say several times before. Please do noy keep closing PRs and issues after only a small amount of discussion.

@simensen

@drak The thing is, the string search thing is an implementation detail of the string search implementation. The autoloader needs to be aware that if it is doing a string search it needs to ensure that the prefix is treated like it has a trailing \. That is the only thing that needs to happen. Forcing the user to always included a trailing \ for things like Dflydev\Canal\ is redundant when the prefix of Dflydev\Canal is, per the specification, supposed to be treated as a namespace. Does that makes sense?

Under no circumstances is Dflydev\Canal ever supposed to match Dflydev\CanalFoo class per the specification, so why should we require the end user to have to register the prefix as Dflydev\Canal\ just because certain implementations might be un-smart and not bother to manually add a trailing \ when they are doing string searches? The trailing \ is redundant as far as I am concerned and will be a nuisance to users.

@ghost

@simensen

Under no circumstances is Dflydev\Canal ever supposed to match Dflydev\CanalFoo class per the specification,

Sure, but this is about the namespace. Autoloader are basically listeners which decide whether to act or not.

$loader1->addNamespace('Foo', '/path/to/Foo'); // A
$loader2->addNamespace('FooBar, '/path/to/FooBar'); // B

So the problem here is loader 1 may respond for FooBar\Printerrequest because it thinks "oh, the class prefix starts with Foo so this is one I know about. It may depend on how you implement, for example exploding an array is one way, but it's not the only way, string comparisons are another.

I am not stating anything new, this is something that is known about and the reason it's even mentioned in Composers documentation: https://github.com/composer/composer/blob/master/doc/04-schema.md#psr-0

@pmjones

As vs. Composer adding a backslash to namespaces on its own, which would be trivial to do (and which we do in the example implementation).

@simensen

The problem I see here is with these implementations. The method name is addNamespace and as such, it should be implied that Foo be treated as namespace and should not under any circumstances match FooBar\Printer. This is a bug in the class loader that should be fixed and addressed.

I meant to say as much here composer/composer#1891 but the PR had already been merged. The fix should have been to ensure that Composer's autoloader treated Foo as a namespace and never matched FooBar\Printer instead of putting that on each package owner to have to declare their PSR-0 prefix as Foo\. It solves the problem, yes, but I think the fix is in the wrong direction. This was a bug in the class loader... it was not a bug in the spec.

@ghost

@simensen - I can see your point but that would mean every PSR0 autoloader in existence is wrong. Clearly the issue is well understood because many big projects are using the trailing backslashes (I might add this is not because of Composer's documentation because they were only recently updated). Evidently projects are doing that because they realised the issue long ago (Zend, Symfony, Doctrine etc). These projects must have realised because they created various packages with similar namespace prefixes, like Doctrine\Common, Doctrine\ORM etc etc/

But you see, even in the current draft spec, it clearly states we use a backslash by example:

  • namespace: Given a fully qualified class name of \Foo\Bar\Baz\Qux, the namespace is \Foo\Bar\Baz\.
  • namespace prefix: One or more contiguous namespace names at the start of the namespace. Given a fully qualified class name of \Foo\Bar\Baz\Qux, the namespace prefix may be \Foo\, \Foo\Bar\, or \Foo\Bar\Baz\.

So either the spec must mandate the trailing backslash (which is does already by implication) or it must clarify the point so implementors can hack in a workaround, like adding a trailing slash internally (which seems ugly imho).

@pmjones

The "workaround" in the general-purpose example is to trim all backslashes when calling addNamespace(). That way the user can add them, or leave them off, as he wishes. I still don't get why you feel that to be a problem.

@ghost

My issue is not with the implementation example, but with the spec. The implementation examples do not form a formal part of the spec. One view is that all PSR0 autoloader are doing it wrong - if that's so, it's the fault of the specification. The specification is not 100% clear because the definitions correctly trailing slashes but there is no clear explanation about why. That is why it should be in the specification somewhere, somehow.

@pmjones

So, to be clear, your specific issue is entirely unrelated to the code examples, and in fact requires no changes to the code examples, and instead is only the modified phrase: " The namespace prefix of a fully qualified class name MUST terminate in \ and MUST be mapped to a base directory ..." -- is that correct?

@ghost

@pmjones - you could say that, but I would have thought that if you mandate that, the code examples need to comply with it. The first closure example already complies but the second you could think about it for a while, needs to enforce it since it's configurable where the first code example is not.

@pmjones

So it is not only about the phrasing, but the phrasing and the related examples. Under the existing implementation examples, the user can call addNamepace('\Foo\Bar\') if they like (which appears to be your preference) or addNamespace('Foo\Bar') if they like (which seems harmless both to me and @simensen so far). So I still fail to see the exact problem here; the case you're worried about seems to be taken care of. What am I missing?

@pmjones

On review of the PSR, the "definitions" section does show examples of what you're talking about. To wit: "namespace prefix: One or more contiguous namespace names at the start of the namespace. Given a fully qualified class name of \Foo\Bar\Baz\Qux, the namespace prefix may be \Foo\, \Foo\Bar\, or \Foo\Bar\Baz\ ."

@simensen

@drak Would it be acceptable to add another section that discusses this in the following way?

An implementation must take care to ensure that a registered namespace prefix such as Foo\Bar will not match for a fully qualified class name Foo\BarQux based simply on a string comparison match of Foo\Bar against Foo\BarQux.

@pmjones I was going to mention about the inconsistency and see you've already addressed it while I'm writing this. :)

@drak All in all, I like that you are trying to look forward on this problem and there is nothing we can do about PSR-0 at this point, but I would like to find a solution to address this problem at the source since PSR-X is new. I think you are wanting to do the same. :) We're currently trying to tackle it from opposite sides... :)

@pmjones

@simensen I think your suggested language might be a good addition. Would you care to bring it up on the list so others can see?

@simensen

I would be happy to but I kinda of want to get feedback from @drak first. If it doesn't address the underlying issue the discussion on the list might be moot and just lead to more back and forth with even more people. :)

@pmjones

Wisdom.

@ghost

Hi guys, it's late here, and I need to test the current implementation to double check before I can answer with certainty. So if possible I'd like to reply tomorrow.

@simensen

@drak Could you weigh in on this so we can bring it to the group if you're OK with it?

@pmjones Would you like to normalize all of the instances of namespaces to not have any prefixed or suffixed \ anywhere in them? For example:

Given a fully qualified class name of \Foo\Bar\Baz\Qux, the namespace prefix may be \Foo\, \Foo\Bar\, or \Foo\Bar\Baz\.

would become

Given a fully qualified class name of Foo\Bar\Baz\Qux, the namespace prefix may be Foo, Foo\Bar, or Foo\Bar\Baz.

I'd be happy to send a separate PR specifically to clean this up if you're busy. Not sure when I'll have time, but I want to help make sure this point isn't lost.

@simensen

Ping @drak @pmjones

I know it has been crazy lately, but I want to make sure this doesn't get lost. It is important to me that people do not need to have to terminate their namespace prefixes with \ due to how this spec is currently written.

So, my goal is to find a way to ensure that @drak is happy with the spec such that a configuration of Foo\Bar for a prefix is all that is required and we do not have to use Foo\Bar\ instead.

For clarity, we are talking about 1) cleaning up the namespace examples to ensure that none of them have trailing (and leading?) \ anywhere and 2) adding the following wording:

An implementation must take care to ensure that a registered namespace prefix such as Foo\Bar will not match for a fully qualified class name Foo\BarQux based simply on a string comparison match of Foo\Bar against Foo\BarQux.

(based on this comment above)

@pmjones originally asked me to post this to the mailing list but I wanted to get @drak input on it before doing so.

@RobLoach RobLoach commented on the diff Jul 18, 2013
proposed/autoloader.md
*/
public function addNamespace($prefix, $base)
{
- $prefix = trim($prefix, '\\');
+ // validate $prefix
+ if ($prefix[strlen($prefix)] !== '\\') {
+ throw new \InvalidArgumentException('Namespace $prefix must terminate in \\');
@RobLoach
RobLoach Jul 18, 2013

Does the example need to actually check this? Seems like if it's in the documentation, we can assume it will be abided.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment