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

The name of the static method 'new' in the Atlas class seems to generate a parse error in third party libraries like phpDocumentor #101

Closed
andrewscaya opened this issue Jan 14, 2019 · 14 comments

Comments

@andrewscaya
Copy link

@pmjones --

I've just had an issue while trying to parse annotations with phpDocumentor within a class that was using the 'new' static method of the Atlas class. The static function 'new' of the main Atlas class seems to be interpreted as the T_NEW PHP symbol and, therefore, generates a parse error. Is there any specific reason why this name was chosen? If not, I would suggest the following change to the main Atlas class:

[...]
    public static function new(...$args) : Atlas
    {
        return static::create(...$args);
    }
    public static function create(...$args) : Atlas
    {
        $transactionClass = AutoCommit::CLASS;
        $end = end($args);
        if (is_string($end) && is_subclass_of($end, Transaction::CLASS)) {
            $transactionClass = array_pop($args);
        }
        $connectionLocator = ConnectionLocator::new(...$args);
        $tableLocator = new TableLocator(
            $connectionLocator,
            new MapperQueryFactory()
        );
        return new Atlas(
            new MapperLocator($tableLocator),
            new $transactionClass($connectionLocator)
        );
    }
[...]

If this makes sense, please let me know and I can send a PR to you.

Many thanks!

Best regards,

Andrew Caya

@andrewscaya andrewscaya changed the title The name of the static method 'new' in the Atlas class seems to generate parse error in third party libraries like phpDocumentor The name of the static method 'new' in the Atlas class seems to generate a parse error in third party libraries like phpDocumentor Jan 14, 2019
@froschdesign
Copy link
Contributor

@andrewscaya
I think you should check your PHP version, because as of PHP 7.0 the new keyword is allowed as method name of a class.

@andrewscaya
Copy link
Author

andrewscaya commented Jan 14, 2019

@froschdesign --

Thanks for your quick response.

I'm running my project on PHP 7.1.24 right now, due to limitations from phpDocumentor and the nikic parse library. The issue arises from the fact that some PHP libraries still seem to have an issue with the presence of the 'new' string being used as a symbol other than the T_NEW keyword.

The proposed solution is simply a desire to give projects the possibility of avoiding to have to extend the Atlas class in order to parse their annotations correctly.

Thanks again!

Best regards,

Andrew Caya

@froschdesign
Copy link
Contributor

@andrewscaya
Sorry, but why should other libraries change their code if the problem is with phpDocumentor and / or the Nikic parser?!

@andrewscaya
Copy link
Author

Was just wondering if there was an architectural reason for choosing this name for this method.

This being said, the issue is also open with phpDocumentor now: phpDocumentor/phpDocumentor#2057

Thriving to make the lives of developers easier is the only possible and ultimate reason behind any decision concerning the architecture of any framework.

Cheers!

@froschdesign
Copy link
Contributor

Thriving to make the lives of developers easier is the only possible and ultimate reason behind any decision concerning the architecture of any framework.

I think therefore the name new was chosen: is simple and clear. 😄

This being said, the issue is also open with phpDocumentor now: phpDocumentor/phpDocumentor#2057

The issue description is wrong, because Atlas is not the problem. The phpDocumentor and / or the used PHP parser can not handle the keyword new, which is allowed in PHP 7 for own (class-)names.

@andrewscaya
Copy link
Author

Not following the line of reasoning here. If you are the project lead, you can do as you see fit, obviously.

Cheers!

@froschdesign
Copy link
Contributor

froschdesign commented Jan 14, 2019

Not following the line of reasoning here.

Please don't get me wrong, I understand your problem. But I don't see the problem here on Atlas or on other libraries which used standard PHP code that allows method or function names like new.

Reference:

As of PHP 7.0.0 these keywords are allowed as property, constant, and method names of classes, interfaces and traits, except that class may not be used as constant name.

http://php.net/manual/en/reserved.keywords.php

If phpDocumentor can not parse standard PHP code, then the problem is on phpDocumentor.

If you are the project lead…

I'm only a contributor – see at the line above comments.

@pmjones
Copy link
Contributor

pmjones commented Jan 14, 2019

Hi @andrewscaya --

Nice to see you here, and I'm sorry for the trouble.

Is there any specific reason why this name was chosen?

Yeah: given $foo = new Foo(); it seemed clear and straightforward to use $foo = Foo::new();. And, as @froschdesign noted, using new as a method name (among other things) is allowed since PHP 7.0.

I feel your pain on PhpDocumentor; I had to give up using it myself over a year ago (maybe 2+ years at this point?) because it has not been able to keep up with the rapid changes in PHP itself. That's why, e.g., I added --no-docs to Producer, because PhpDocumentor kept erroring on valid code structures. This seems like yet another of those problems; that is, it's not Atlas at fault here.

Having said all that, I don't want you to have to endure any more hassle than you have to. You say "in order to parse their annotations correctly" which makes me think you're parsing a project to read docblock annotations. Can you tell me more about that? Perhaps there is some way around PhpDocumentor's incorrect behavior.

@andrewscaya
Copy link
Author

andrewscaya commented Jan 14, 2019

I understand what you are saying. But, I firmly believe that perfect code is only perfect if it is truly useful to those that use it every day. My suggestion is given with the intent of making sure that developers are NOT turned off by a detail that can be easily fixed with some wrapper code.

This being said, this issue can be closed if it is not deemed useful.

All the best to Atlas ORM! :)

Cheers!

@andrewscaya
Copy link
Author

andrewscaya commented Jan 14, 2019

Hi @pmjones!

Nice to be here! :)

I'm including Atlas ORM in a small MVC framework that I am developing for my University students (will also be shared publicly for those that are interested). My problem is already fixed as I have wrapped the method call with a name that is not causing the issue.

This being said, I like this project very much and I will be teaching it in my class on MVC frameworks at Concordia University. I was only trying to be helpful.

Many thanks for taking my issue into consideration!

Cheers!

Andrew Caya

@andrewscaya
Copy link
Author

@pmjones --

Is there any way I could reach you personally? Tried your Twitter account, but I can't seem to find it anymore.

Many thanks for your time!

Best regards,

Andrew

@pmjones
Copy link
Contributor

pmjones commented Jan 14, 2019

Absolutely -- I am pmjones at pmjones dot io. (Deleted the Twitter account some months ago.)

@froschdesign
Copy link
Contributor

froschdesign commented Jan 15, 2019

@andrewscaya
Please try the alpha version 3 of phpDocumentor, because version 2 will not support the allowed keywords:

Phpdocumentor is now able to parse files using php 5.5+ syntax, but will not process any of the new key words to documentation.

https://github.com/phpDocumentor/phpDocumentor2/releases/tag/v2.9.0

See also at this issue and comment: phpDocumentor/phpDocumentor#2055 (comment)

I hope this will work for your project!

@andrewscaya
Copy link
Author

@froschdesign Many thanks for taking the time to check it all out for me! :)

@pmjones pmjones closed this as completed Jan 30, 2020
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

3 participants