Skip to content

Importing and aliases vs. fully qualified namespaces #872

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

Closed
ldusan84 opened this issue Dec 25, 2014 · 5 comments
Closed

Importing and aliases vs. fully qualified namespaces #872

ldusan84 opened this issue Dec 25, 2014 · 5 comments
Labels
improvement Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development

Comments

@ldusan84
Copy link
Contributor

I think that Magento2 should import namespaces and use aliases instead of using fully qualified namespaces all over the place.

Here is an example.

What I suggest is that instead of:

throw new \Magento\Framework\Model\Exception(
    __('Invalid login or password.'),
    self::EXCEPTION_INVALID_EMAIL_OR_PASSWORD
);

We could have:

use \Magento\Framework\Model\Exception as MagentoException
/* ... */
throw new MagentoException(
    __('Invalid login or password.'),
    self::EXCEPTION_INVALID_EMAIL_OR_PASSWORD
);

Here are the benefits from this:

  1. With importing namespaces keyword at the first few lines of a class you actually get a nice documentation of your class dependencies. Everything that class uses is in those first few lines. It makes dependencies explicit.
  2. If something changes in the future, you would only need to change it in one place, in import declaration and the alias remains. With fully qualified namespaces you would need to find them everywhere in class and change.
  3. Less confusion if you have similar namespaces, you can give them different aliases.
  4. All major frameworks use importing/aliases over fully qualified namespaces.

Let me know what you think.

@orlangur
Copy link
Contributor

I totally second the importing approach 👍

use \Magento\Framework\Model\Exception as MagentoException
Less confusion if you have similar namespaces, you can give them different aliases.

I would rather forbid aliases usage or allow them only for exceptional cases. Proper class name must contain all data necessary to understand the purpose of class. There are a lot of names with directory-per-word approach and would be better to rename classes than introduce aliases each time you want to use them, for instance:
Magento\Framework\Model\Exception -> Magento\Framework\Model\ModelException
Magento\Catalog\Model\Resource\Product\Collection -> Magento\Catalog\Model\Resource\ProductCollection
So that classes could be imported without aliasing.

You may notice that there are a lot of code in Magento2 using short names already. But in code which was not changed for a long time there are still a lot of fully-qualified class names. Would be nice to replace fully-qualified class names with imports all over the code and enforce short name usage with static test (for example, check that T_NS_SEPARATOR token does not appear outside the namespace and use statements; or some more sophisticated check if we want to allow relative class names).

What tool would you suggest for such transformation? The closest ready-to-use utility I found so far is PHP Refactoring Browser, but "optimize use statements" feature here does not work as expected: type-hinted class names in constructor/other methods, PHPDoc blocks are not updated.

So, except the IDE abilities which I didn't analyze, I would build such script using one of:

@ldusan84
Copy link
Contributor Author

Thanks @orlangur for the comment.

I just picked that Exception class for no reason, and yes I should have made a better alias, but for me even importing without alias is still better than fully qualified name. Maybe aliases could be used only if we have two classes with the same name.

To tell you the truth, I am not sure which tool would be the best to do this automatically. I know some IDEs can do that, but I am not sure how reliable are they. I guess it would be hard to find a reliable tool.

@verklov
Copy link
Contributor

verklov commented Jan 5, 2015

Internal ticket: MAGETWO-32347

@vpelipenko vpelipenko added the Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development label Apr 21, 2015
@vkorotun vkorotun removed the PS label Aug 4, 2016
@piotrekkaminski
Copy link
Contributor

Thank you for your submission.

We recently made some changes to the way we process GitHub submissions to more quickly identify and respond to core code issues.

Feature Requests and Improvements should now be submitted to the new Magento 2 Feature Requests and Improvements forum (see details here).

We are closing this GitHub ticket and have moved your request to the new forum.

mmansoor-magento pushed a commit that referenced this issue Feb 26, 2017
[Performance] Performance Profile Generator
magento-engcom-team added a commit that referenced this issue Sep 7, 2019
…nloadableCart… #872

 - Merge Pull Request magento/graphql-ce#872 from Rus0/graphql-ce:ISSUE-824
 - Merged commits:
   1. e20be33
@dresh
Copy link

dresh commented Jan 9, 2020

use PHPstorm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development
Projects
None yet
Development

No branches or pull requests

8 participants