-
Notifications
You must be signed in to change notification settings - Fork 7.7k
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
[PHP.NEXT] Add support for package private class, trait and interface #947
Conversation
if (!( | ||
strrchr(ce->name->val, '\\') != NULL && | ||
strrchr(parent_ce->name->val, '\\') != NULL && | ||
strncmp(ce->name->val, parent_ce->name->val, strrchr(ce->name->val, '\\') - ce->name->val + 1) == 0 |
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.
hmm, classname are case-insensitive in php, so maybe zend_binary_strcasecmp here?
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.
@laruence first time I saw this function... yes, it makes sense. I'll update the patch with this usage. =)
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.
Maybe it's time to call for case-sensitive classname... After all, universal ClassLoader will appreciate it.
\Namespace\User; src/whatever/Namespace/User.php !== src/whatever/Namespace/USER.php
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.
@kinncj: That would be a massive BC break.
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 why it's called "new generation" :-)
And yes, I agree... it will be.
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 might break if internally it uses case-insensitive to differentiate class names. However, only people who really know how PHP would consider problems. For instance, some filesystems, e.g., EXT4, allow files named User.php
, uSer.php
and user.php
respectively to exist in the same folder. How would developers implement autoloader to pick? Which one is the chosen one?
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.
@kinncj No, that's not so good idea. :-(
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.
@Kubo2 , if it's not related to BC break, can you explain why? (only if it's not related to BC break).
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.
@kinncj No, it isn't related to BCb. But in PHP world, there is a big number of programmers, who does not all have the same conventions (especially if one writes Class_With_Underscored_Name, other ClassWithCamelCasedName but someone else Classwithfulllowercasenameexceptfirstcharacter) and if you are instantiating such third-party classes, it is a good thing to at least partially keep your own or your core's conventions (because the code looks much better).
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.
Extremely disagree.
PHP is evolving, and developers _must_ do so...
It was the same with the regex \e
operator or even ereg_*
It's natural that Class_With_Underscored_Name
represents a
package name and folders + class name are logically uppercase, as @shiroyuki
said: . "For instance, some filesystems, e.g., EXT4, allow files named User.php
, uSer.php
and user.php
respectively to exist in the same folder. How would developers implement autoloader to pick? Which one is the chosen one?"
The same works with ClassWithCamelCasedName
Nowadays vendors are really committed to quality, we are not talking about
orphan libraries in PEAR...
Anyway, it's out of scope of this PR.
Nice. Do you also plan to add some support to ReflectionClass? a) visibility reflection:
b) accessibility reflection:
|
3910872
to
46fe83f
Compare
} | ||
|
||
namespace C { | ||
class D implements \A\B { |
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.
You may want to add new tests for these cases that involve class autoloading:
spl_autoload_register(function () { eval('namespace C { class D implements \A\B {} }'); });
749ba64
to
0fff196
Compare
|
Only same namespace. As I said earlier, it is inline with package private from other languages.
This reasoning requires a deeper explanation. Consider a public Factory class inside of a namespace with many private classes. Upon instantiation, Factory will instantiate the private class which can be a dependency of another class outside of that namespace. It is perfectly fine to consume a private class outside of its namespace. |
@guilhermeblanco: Is there a reason for not introducing protected visibility? I see some use cases for it, e.g. when splitting a class/subsystem to a smaller components, which should stay internal (object composition) One more question, how would you propose to properly test private classes? Tests usually reside in a completely separate namespace (e.g. |
Why would classes outside current namespace depend on a |
@Majkl578 you can test private classes by using ReflectionClass::setAccessible. |
@Kubo2 that would be the first language to have such thing. It also would "-" to become a valid identifier which can be a BC break and also 2 new keywords (another potential BC break) |
No, Java has something similar, I think, but they implemented it as „nested classes“. I think potential package-internal implementation in PHP would be more descriptive than the Java's one.
Why? It could be like a
I don't see a BC break here, exactly. But if so, PHP 7 is a „new generation“ and thus BC breaks are welcome, aren't they? :-) I would try to implement it, when I learn C one time. |
@Kubo2 , how can you contradict your self?
In the end, yes: making it case-sensitive is a BC, and will eventually break some vendors... That's what you said today:
How can you be against case-sensitive in php-ng, and ask for an FYI: Internal occurrences Without case-sensitive you may break things like: class Internal {}
interface Internal {}
define ("Internal", "foo");// Nice, you can define, but not use...
function Internal() {}
const Internal = "foo";
/** etc, etc, etc... **/ |
Yes, you can use it, with only restriction, that you can not directly
Ah. Yes, you are right. It is really not as good idea as I thought. But instead of introducing
Sorry for that. Few days passed, I have read another large amount of pull-requests for PHP 7 during that time and on that base I little changed my mind.
If so, I am no longer against implementing case-sensitive indentifiers in PHP (but thus for introducing package-internal visibility, as that could be the solution for it — following on this). |
ce99e02
to
8762300
Compare
@guilhermeblanco I haven't seen any activity on this in internals. Is this something you still want to work on and get RFC'd before the feature freeze? How can I help? :) |
@lt I need to address one bug that is still active and the RFC that needs to be written. |
I will draft the RFC today and submit to you + a couple of others this evening (UTC :)) for review. |
Just thought of something. |
I see tests for expected failures during autoloading, but none for successful autoloading. |
8762300
to
8c4a2e6
Compare
@flaupretre Having a @guilhermeblanco Do you still intend to go forward with this? |
I'm looking at your examples for better understanding of your feature and I think there is a missconception, for eg. if we have:
Providing such private class can cause missconcept usage out of my package - which is |
I think a private class is more like a private inner/nested class, whose instantiation and usage are restricted by the outer class. What you are proposing is more like an internal class in C#, although C#'s internal keyword is to restrict usage within an assembly, not just one namespace. |
@brzuchal It is exactly what your example described. |
Ok so how about example with library. Assuming I'm develoing complex library with other 10 developers and need to hide my specific class drivers to awoid misusage:
Can it be handled properly? I'm not assuming that package-private classes in PHP should work exactly like in Java because there they are causing plenty of classes in same package/namespaces. I want to keep the right order and place my classes in folders. |
@brzuchal Personally I can't remember if I allowed sub-namespaces or parent-namespace access. |
It is correct that <?php
assembly brzuchal {
namespace MyVendor\MyLibrary {
public class PublicApi {
public function __construct() {
$this->widget = new InternalStuff\NonPublicWidget();
}
}
}
namespace MyVendor\MyLibrary\InternalStuff {
class NonPublicWidget {}
}
} |
@Fleshgrinder I was thinking about the same but called it |
From my mailing list messages:
|
Just noticed your proposal to combine package and namespace. This is actually a very nice idea and would help to keep repetition low. How feasible it is would be another question but I really dig the idea. |
@Fleshgrinder Yes I wanted to use package instead of namespace to provide this what you calles assembly (that keyword is also fine for me) but that's no further purpose of this PR. I'll keep reading internals at your thread and try there to continue discussion. Have you got any implementation started yet? |
Nope, mainly because I need time to get into all the Bison stuff. |
@guilhermeblanco What is the status of this feature? When can I expect it could be in PHP? |
@brzuchal First, let's talk about what is left:
On first part, I hit a dead-end. If we take the first approach, it's an entire RFC itself and could potentially invalidate all the implemented approach took here (PHPT tests, BNF would still be usable though). Also, I have no extensive knowledge of all internals tricks... onboarding any approach would be steep learning curve for me. Also, I have academic knowledge in C, which is good enough to read code, but not to do extensive writes of new functionality. =( The second part should be ok on itself. The third part I haven't put any thoughts, but conceptually I envisioned it doing something similar to "private", but also I would like to see the ability to access protected members of classes (properties and methods). As said before, I didn't invest a lot of time in this because it could easily be another RFC after this one. |
@guilhermeblanco It's been a while since I last looked at this, so not sure if I remember things right. Some comments on the namespacing issue: First, note that there may be control-flow that crosses namespace boundaries in the form of jumps. For example, this is valid code: namespace A {
echo "In A";
goto label;
echo "Still in A";
}
namespace B {
echo "In B";
label:
echo "Still in B";
} The This leaves your other suggestion of representing namespaces in their own structure. In principle this should be relatively simple: Introduce a |
@guilhermeblanco this generated a very decent amount of conversation, there is obviously much interest in the feature. It also provoked other contributors to raise concerns and they haven't yet been addressed by comments or updates to the patch: Can I ask for an update on the status of this RFC please ? If you consider this work abandoned, please close this PR yourself. |
I hate this, but since it's been 6 months since I asked for feedback, and since there are unresolved issues in this very out of date implementation, I'm going to close this PR :( Please please please take this as encouragement to open a clean PR, and get the RFC process started. |
@krakjoe This feature is not possible without severe engine tinkering. I have some ideas on how to resolve the prerequisites but it will involve changes to opcache - a blocker for many of us. When code in the top level scope can be aware of the namespace it is executing in, we have grounds to re-open :) Thanks for attending to this issue! |
I have some serious intrest in this feature, I come from the rfc page (last update atm is 22-09-2017(dd-mm-yyyy)) and this PR is musch more out of date. Is there a place where this feature request is still alive? |
Can we revive the discussion? |
As @lt said:
This isn't done so there are no chances. |
Or another approach is to forbid top-level code when visibility is used. We don't really need top-level code in namespaces, we only need it in one single file per project: the bootstrap. |
Looks like it's back by @mdwheele https://wiki.php.net/rfc/namespace-visibility |
This is fantastic that someone is willing to raise up the subject again.
Last time there was a question about how it should work with goto:
<?php
namespace Example {
private class A {}
$a = new A();
goto out;
}
namespace OtherVendor {
out:
var_dump($a);
}
This is a valid PHP code and the question is should there be no access to
$a inside OtherVendor namespace?
An additional question is what about functions? Because this RFC is all
about classes only.
I keep my fingers crossed for this feature.
That might be a reason to update codebase from 7.2 because now I don't see
that much to upgrade to 7.3 at all.
niedz., 9 wrz 2018 o 04:00 Gabriel Caruso <notifications@github.com>
napisał(a):
… Looks like it's back by @mdwheele <https://github.com/mdwheele>
https://wiki.php.net/rfc/namespace-visibility
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#947 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ADAPuWNrKwUcVKivB_6ef-Ce9TPaM3LEks5uZHZQgaJpZM4DF47g>
.
|
It should not work, for obvious reasons. Nor should the current implementation work (which it unfortunately does...). If I declare something in a namespace, I expect the variabled to be namespaced too, not written to the global scope. I hope someone picks up this pull request, it would be a really great feature to have! #revivingdeadpullrequests |
Another interesting feature this PR could make way for is namespace-private class properties, for example using the "internal" keyword (as in Swift: https://docs.swift.org/swift-book/LanguageGuide/AccessControl.html). |
I really want to see progress in this RFC, because, despite the presence of |
Consumes PR #931
This PR allows this to be possible:
To not be allowed as library developer of Foo may not have planned extension points in class
Bar
.RFC to be written yet...
Implementation progress:
ReflectionClass::isPublic()
andReflectionClass::isPrivate()
ReflectionClass::setAccessible()
Add more tests coverage for:
ReflectionClass:
ReflectionClass::isPublic()
with implicit valueReflectionClass::isPublic()
with explicit valueReflectionClass::isPrivate()
with explicit valueReflectionClass::setAccessible()
Classes:
Interfaces:
Traits: