John, On Tue, Mar 6, 2012 at 9:04 PM, John Crenshaw <johncrens...@priacta.com> wrote: > Disclaimer: The following is direct (maybe brutally so). I'm not trying to > hurt any feeling or attack, but I'm not pulling punches either. I don't have > the energy right now to polish this and make it all nice and gentle, so I'm > sorry in advance. I hope you'll look past the directness and be able to get > some useful feedback.
Thank you for being candid. I appreciate straight to the point replies, as long as they are respectful (which yours was very much so). So thank you. Now, to your points. > Yes, it was aimless discussion (though some would call it brainstorming). It > was ready to move to the next level, which should have been gathering the > constraints/requirements/problems into a coherent list to work from. What > happened though was that this step was skipped entirely, so now there's been > an attempt to write a preliminary spec without gathering the requirements > first, which IMO is far worse than any amount of aimless discussion. With all honesty, I didn't see anything being gathered. What I saw was random ideas shot back and forth (with a good bunch ignored in all the chatter). There didn't really seem to be one or two clear directions, but instead at least a dozen directions at the same time. There was talk about implementing a "scalar" hint, about implementing strict hints again, about implementing pseudo-casting hints that did only simple checks, about hints that actually casted, about hints that threw notices, about hints that threw warnings. There didn't seem to be anything more than a bunch of people wanting different things not even paying attention to the other conversation. Could a spec have been developed from that? Sure. Would it have? I'm not so sure. So I picked a direction that seemed consistent with as much of it as possible, and went with it. I didn't ask anyone to stop other work, or stop their discussion. I just saw an opportunity to unify the conversation around what seemed to be a commonality between the comversation, and I took it. If I stepped on anyone's toes, I'm sorry... > A good number of issues with the current proposal were raised during the > discussion on the mailing list. I don't feel like digging them all up right > now, but off the top of my head I remember the following being raised and > never saw any consensus for how to resolve them: I went over the replies to the initial POC thread that I posted (http://marc.info/?t=133066037200001&r=1&w=2) and I'll rebut your replies. > - inconsistent syntax (one syntax for scalars, a different one for classes) This is actually discussed in the RFC, as it is not inconsistent (it's actually consistent with what the patch tries to achieve). The syntax for classes and normal arrays is a strict check, where if the match fails an error is thrown. This syntax attempts to distinguish between that functionality by providing a different syntax altogether. And since it's casting the parameters, the syntax feels natural (for that goal). > - conflicting syntax (I.E. array vs. (array), RFC simply "allows" this, and > ignores the confusion that this will create for users.) Actually, it doesn't simply allow that. It did it for a very specific reason. "array" is a strict check, and "(array)" is a casting check. One will fatal if a non-array is passed, and the other will attempt to convert the parameter to an array. Very different functionality, which are both internally consistent with the other syntax... > - different from the syntax used in the docs Actually, it's the exact same syntax used for casting in the docs. It's different from class type hints, because it's intended to be so. If you don't like it, that's fine. But it's intentionally different. > - lack of sufficient function to justify a core change That's absolutely something to be considered. However, I see erroring on invalid casts as a bigger issue not the responsibility of a **casting** hint patch. So that's why I mention explicitly in the RFC and my blog post that solving that problem should be another RFC (culminating in a series of 3 RFCs that each work together very well to fill the overall need). > - chaos surrounding null (to accept and if so to cast or not? Creates a > conflict between consistency and implications of the syntax) That's absolutely a valid concern. And that's what should be being discussed, if people really feel that the current implementation is wrong... > - conflicts with references (RFC tries to address this by simply disallowing > references, which IMO just ignores the need that would have caused this sort > of code in the first place.) Actually, it explicitly disallows references. This was added because it was explicitly requested on the list in many discussions (including the initial one for the POC). But if you can make a case on why to implement it, and why it makes sense, then we can add it back. > There were others, but I'm not making an exhaustive list. Well, the ones that I saw were either rectified, requested (and implemented) or (to me) showed a lack of understanding about the rationale and decisions (which is valid, but shouldn't really bear that much on the discussion, as I feel if you want to raise an issue, you should at least have a cursory understanding of what you're commenting about)... >> As far as it being crippled, I'm not sure what you mean, just because it's >> only doing casting? >> > > Yes, casting is barely better than doing nothing. If I want a dumb typecast I > can do that already. What I can't do without massive boilerplate everywhere > is an intelligent conversion that accepts safe conversions and gives a > warning or error on unsafe conversions. Again, I personally see casting data-loss a bigger issue than just parameter hinting, which should get its own RFC to clean up. That's why I didn't include it here. On purpose... >> As far as confusing, it is? I thought this was actually one of the more >> straight forward proposals, since it re-used so much from the core (meaning >> that it doesn't add new behavior, it re-uses existing behavior). >> > > Yes, the syntax has some critical issues that create conflicting > expectations. Look at the prior discussion. The current proposal doesn't > really fix most of this. Conflicting expectations? It looks like a cast to me, and that's what it does. I'm not sure where the conflicting expectation is. I would agree if it was foo(int $int) that did the casting. That would be conflicting. But with the current syntax, I'm not sure I understand your point... >> As far as having a plethora of unadressed issues, I'm absolutely sure it >> does. But I haven't seen a single one put out there so that it can be >> fixed... >> > > Again, look at the prior discussion of this syntax. Plenty of issues raised. Actually, I did. And the only issue raised was by you here: (http://marc.info/?l=php-internals&m=133066473032680&w=2). And I believe those issues were addressed in the RFC. If you don't feel that's adequate, that's fine, let's improve the RFC so that it does... > The biggest one that comes to mind is behavior with respect to operators. > __toScalar() in your spec is an attempt to handle this, but IMO it really > doesn't cut it. Off the top of my head problems with this solution include: > - This will lead to duplicated code It will? Yes, __toInt() and __toScalar() may have the same code in it. But that's always going to happen, unless you want to make "+" an integer operation. But you could just do __toScalar() { return (int) $this; } to take care of that duplication. The key is that it leaves the determination of the type to return to the class, where it belongs... > - Thinking about my own code, I have almost no idea how I would actually > write a robust __toScalar() implementation. It's still going to be returning > a string, or an integer. Very confusing. You probably won't implement it at all then. I admit, the __toScalar() method is the weak point of the RFC. I personally want the flexibility that it provides, but if people are strongly against it, I think it can be removed without destroying the RFC's intent... > Well, the big issue for me is that I think the type casting hints starts from > a fundamentally bad foundation. The premise here is based on a syntax that is > ugly and strange, and the syntax opens a bunch of new problems that I don't > see a way to resolve. The reason for selecting this syntax was to avoid a BC > break from reserving some new keywords. I don't really see this as a good > trade. To be perfectly clear, the reason for that syntax was **not** to avoid a BC break. It was to avoid a FC break. I wanted a casting hint, and I didn't want to restrict the possibility of adding strict hints later. And I wanted to clearly distinguish the current strict hints (array + class) and the new casting hints. To me, the most logical way of doing that was to re-use the casting syntax. Could we have done foo(int? $foo) or foo(*int $foo) or foo(@int $foo) or whatever? Sure. But I'd argue that anything except foo(int $foo) would have lead to FAR more confusion. This way, it's re-using an existing syntax that **every** intermediate developer is intimate with, and applying it to a case that's basically functioning along the same lines... > As for the automatic casting functions, I like the concept, but some serious > problems were raised in the discussions and many were never resolved very > well. To some extent I'm not really seeing good solutions either. If I think > of a solution I'll be sure to mention it, but right now this looks very much > like a dead end to me. Actually, there weren't. This is the thread you were referring to: (http://marc.info/?t=133026833200004&r=1&w=2). And re-reading it, the problems that are raised are all centered around __assign() (and ever so slightly the scalar casting now implemented as __toScalar()). The rest of it was barely discussed, and no real problems were raised. I'm not saying this to say that there are no problems. I'm saying that, as far as I've seen, no significant (non-personal-preference) issues have been raised about the topics both of these RFCs discuss (with the possible exception of __toScalar(), to a far lesser extent). The topics that were objected two in those threads have been rectified or eliminated altogether from these RFCs. So, again, I ask. Let's look at the actual issues, so we can make these RFCs better (or show that they need to be thrown out). I want the best overall outcome, and if that means ditching my concepts altogether, I have no problem with that. I just don't want to see them ditched because of invalid pre-conceived notions, or because of miss-communication. Thanks for the replies! Anthony -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
