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

fix. extend namespace #1386

Closed
wants to merge 1 commit into from
Closed

fix. extend namespace #1386

wants to merge 1 commit into from

Conversation

nyufeng
Copy link
Contributor

@nyufeng nyufeng commented Oct 30, 2018

Description
Pages extends CodeIgniter\Controller replace to Pages extends \CodeIgniter\Controller
CodeIgniter\Controller class in root namespace

Checklist:

  • Securely signed commits
  • Component(s) with PHPdocs
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

`Pages extends CodeIgniter\Controller` replace to `Pages extends \CodeIgniter\Controller`
@jim-parry
Copy link
Contributor

@lonnieezell What convention should we be following?
Some of the examples in the user guide use namespaces (App\Controllers) while others don't.
Some extend "\Codeigniter..." while others extend "CodeIgniter...".

@lonnieezell
Copy link
Member

That's because, in part, I didn't think it was necessary to repeat the full structure every time, but maybe that is a better practice.

  • All controllers should have the App\Controllers namespace
  • I would always have a use statement above the class: use CodeIgniter\Controller
  • so only ever have to MyClass extends Controller

@jim-parry
Copy link
Contributor

There are a bunch of instances where this needs to be corrected, and following Lonnie's suggestion, not the path taken by this PR. I am closing this, and will make a sweep of the user guide looking for these changes.
Thank you @Instrye for bringing this to our attention :)

@jim-parry jim-parry closed this Nov 1, 2018
@nyufeng nyufeng deleted the patch-1 branch December 5, 2018 03:47
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

Successfully merging this pull request may close these issues.

None yet

3 participants