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

Deprecate TableRegistry static API. #10933

Merged
merged 22 commits into from Feb 20, 2018

Conversation

@robertpustulka
Member

robertpustulka commented Jul 21, 2017

There were some complaints about CakePHP relying too much on static classes and TableRegistry was one of the examples. Actually TableRegistry is just a static API wrapper for TableLocator.

The best practice is to use LocatorAwareTrait in classes that need access to the table "registry".

For example:

//deprecated call
$users = TableRegistry::get('Users');

//use table locator instead
use \Cake\ORM\Locator\LocatorAwareTrait;
...
$locator = $this->getTableLocator();
$users = $locator->get('Users');

//or in global context:
$locator = TableRegistry::getTableLocator();
$users = $locator->get('Users');

TableRegistry::getTableLocator() needs to stay for a while so the apps relying on a global locator wouldn't break.

Nothing changes for classes that use ModelAwareTrait (controllers, shells, etc):

$this->loadModel('Users');
$this->Users->get(1);

@robertpustulka robertpustulka added the ORM label Jul 21, 2017

@robertpustulka robertpustulka added this to the 3.5.0 milestone Jul 21, 2017

@codecov-io

This comment has been minimized.

codecov-io commented Jul 21, 2017

Codecov Report

Merging #10933 into 3.next will decrease coverage by 0.02%.
The diff coverage is 78.94%.

Impacted file tree graph

@@             Coverage Diff              @@
##             3.next   #10933      +/-   ##
============================================
- Coverage     92.12%    92.1%   -0.03%     
+ Complexity    13259    13258       -1     
============================================
  Files           462      462              
  Lines         34053    34061       +8     
============================================
  Hits          31372    31372              
- Misses         2681     2689       +8
Impacted Files Coverage Δ Complexity Δ
src/ORM/Table.php 93.84% <ø> (ø) 285 <0> (ø) ⬇️
src/Console/Shell.php 92.01% <100%> (ø) 98 <4> (-1) ⬇️
src/View/Form/EntityContext.php 98% <100%> (ø) 85 <0> (ø) ⬇️
src/TestSuite/TestCase.php 91.56% <100%> (+0.03%) 79 <0> (ø) ⬇️
src/Auth/BaseAuthenticate.php 92.3% <100%> (ø) 19 <0> (ø) ⬇️
src/TestSuite/Fixture/TestFixture.php 86.06% <100%> (ø) 69 <0> (ø) ⬇️
src/ORM/TableRegistry.php 77.41% <40%> (-22.59%) 12 <0> (ø)
src/Http/Session/DatabaseSession.php 87.23% <66.66%> (-1.66%) 15 <0> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fdfd026...d7cf972. Read the comment docs.

@lorenzo

This comment has been minimized.

Member

lorenzo commented Jul 21, 2017

Don't we end up with the same implementation by making TableLocator a singleton again?

@robertpustulka

This comment has been minimized.

Member

robertpustulka commented Jul 21, 2017

TableLocator has always been a singleton by default.
You can swap locator instances locally though.

@lorenzo

This comment has been minimized.

Member

lorenzo commented Jul 21, 2017

well, it is nice that we are getting rid of the unnecessary indirection

@saeideng

This comment has been minimized.

Member

saeideng commented Jul 21, 2017

I am ok by change way
But not by this
👎
I think we can use shorter naming
This is very long and not good
Please open issue before implement it

@robertpustulka

This comment has been minimized.

Member

robertpustulka commented Jul 21, 2017

@saeideng We can't change the naming without breaking the BC. Table locator related code has been around for a while and is actually the preferred way of locating tables (contrary to calling TableRegistry statically).

@dereuromark

This comment has been minimized.

Member

dereuromark commented Jul 21, 2017

@saeideng It is pretty much the same name length, there really is no overhead here either way.

@ADmad

This comment has been minimized.

Member

ADmad commented Jul 21, 2017

@saeideng At least until 4.0 you can keep using $this->tableLocator(). After that we'll just have to bite the bullet and get used to typing extra "get", "set" for lot of methods. 🙂

@saeideng

This comment has been minimized.

Member

saeideng commented Jul 21, 2017

I looking for very simple way like
$this->getTable('post')

@robertpustulka

This comment has been minimized.

Member

robertpustulka commented Jul 21, 2017

You can create your own trait with a wrapper method.

I'm afraid we can't add getTable() to LocatorAwareTrait since it's a very common method name and could break userland code.

BTW you can still use $this->loadModel('Posts') from ModelAwareTrait.

@saeideng

This comment has been minimized.

Member

saeideng commented Jul 21, 2017

Or
Cake\orm\table:get('posts')

@robertpustulka

This comment has been minimized.

Member

robertpustulka commented Jul 21, 2017

@saeideng Static calls is what we're trying to avoid here.

@saeideng

This comment has been minimized.

Member

saeideng commented Jul 21, 2017

My class name is for v4 😃

@@ -183,7 +183,7 @@ public function __construct(ConsoleIo $io = null)
}
$this->_io = $io ?: new ConsoleIo();
$locator = $this->getTableLocator() ? : 'Cake\ORM\TableRegistry';
$locator = $this->getTableLocator() ? : TableLocator::getInstance();

This comment has been minimized.

@AlaaAttya

AlaaAttya Jul 21, 2017

why not using empty statements instead use this

$locator = !$this->getTableLocator() ?? TableLocator::getInstance();

This comment has been minimized.

@markstory

markstory Jul 22, 2017

Member

We can't use ?? because of php5 compatibility.

@inoas

This comment has been minimized.

Contributor

inoas commented Jul 21, 2017

@saeideng We can't change the naming without breaking the BC. Table locator related code has been around for a while and is actually the preferred way of locating tables (contrary to calling TableRegistry statically).

We can actually with class alias and deprecation.
Why not getTable()? within the trait?

I don't think there should be much of a Problem. If you extended core classes in the past you would continue using TableRegistry::get().

@saeideng At least until 4.0 you can keep using $this->tableLocator(). After that we'll just have to bite the bullet and get used to typing extra "get", "set" for lot of methods.

Well TableLocator sounds pretty much like an object that is mostly used to retrieve state not to set it. So we could just omit the get prefix (but not the set prefix) here.

@markstory

This comment has been minimized.

Member

markstory commented Jul 22, 2017

I confused about why we would add a new trait. What does it do better than ModelAwareTrait does? It appears to do very similar work.

*
* @return \Cake\ORM\Locator\LocatorInterface
*/
public static function getInstance()

This comment has been minimized.

@markstory

markstory Jul 22, 2017

Member

The singleton doesn't remove static class usage it just moves it. This class would now have a mixture of static and non-static state, which doesn't seem great.

This comment has been minimized.

@robertpustulka

robertpustulka Jul 23, 2017

Member

This PR aims to reduce the number of static calls in userland code, to discourage from using TableRegistry and move to the underlying implementation of table locators.

The static call is done only once when the getTableLocator() is called from an objecy using the trait.

This comment has been minimized.

@markstory

markstory Jul 24, 2017

Member

We have ModelAwareTrait already though. How is this better than that?

This comment has been minimized.

@robertpustulka

robertpustulka Jul 24, 2017

Member

Note that I haven't added any new trait. Just reused the existing one.

ModelAwareTrait is used to work with models in general while there were two ways to work with ORM while developing plugins for example. Using static classes shouldn't be the preferred one.

@robertpustulka

This comment has been minimized.

Member

robertpustulka commented Jul 24, 2017

@inoas, @markstory

Note that LocatorAwareTrait is not a new trait. It has been in the core since 3.1.0 and used throughout the core classes as a replacement for TableRegistry "static" class. I think we should have deprecated static calls back then though. This PR does exactly this now, so we can remove TableRegistry class safely in 4.0.

We also can't change trait method definitions here without breaking the BC. Adding a new method with such a common name as getTable() to the trait that is used in base Controller and Shell classes can cause problems with existing applications.

@inoas there are no TableRegistry::get() calls in the core apart from one omission in EntityContext class and tests.

@robertpustulka

This comment has been minimized.

Member

robertpustulka commented Jul 28, 2017

Is there something left to be done here?

@alexsmonte

This comment has been minimized.

alexsmonte commented Aug 4, 2017

One of the great advantages of cakephp is precisely its easy writing, this lowers the learning curve.
I understand the question of static methods however it is necessary to have something simple, as it always was in cakephp.

@robertpustulka

This comment has been minimized.

Member

robertpustulka commented Aug 4, 2017

Is calling TableRegistry::get('Foo') really much simplier than $this->getTableLocator()->get('Foo')?

@saeideng

This comment has been minimized.

Member

saeideng commented Aug 4, 2017

Yes
Because you need to call trait namespace

@robertpustulka

This comment has been minimized.

Member

robertpustulka commented Aug 4, 2017

@saeideng don't you need to use Cake\ORM\TableRegistry on top of your class as well?

@saeideng

This comment has been minimized.

Member

saeideng commented Aug 4, 2017

see usage
1 )linear
//TableRegistry

class A{
\Cake\ORM\TableRegistry:get()
}

//LocatorAwareTrait

class A{
use \Cake\ORM\Locator\LocatorAwareTrait;
$this->getTableLocator()->get('Foo');
}

2 )
//TableRegistry

use Cake\ORM\TableRegistry;
class A{
TableRegistry:get()
}

//LocatorAwareTrait

use Cake\ORM\Locator\LocatorAwareTrait;
class A{
use LocatorAwareTrait;
$this->getTableLocator()->get('Foo');
}

@robertpustulka

This comment has been minimized.

Member

robertpustulka commented Aug 4, 2017

@saeideng looks like for the cost of 1 line you've saved your code from a hard dependency and a global 😃

@inoas

This comment has been minimized.

Contributor

inoas commented Aug 4, 2017

Well the point stands nevertheless. The question is how bad that dependency is when you can easily(!) just use another use statement to swap out TableRegistry.

Edit: Internally this is good by default.

@dereuromark

This comment has been minimized.

Member

dereuromark commented Aug 4, 2017

This looks good.
Whoever wants can still use static access as outlined from here on in project level :)
So this is for now mainly core relevant and helps to overcome the CakePHP stigma of too much static calls around locators and factory access.

@robertpustulka

This comment has been minimized.

Member

robertpustulka commented Aug 4, 2017

@inoas to change the dependency of a static class you need to change your class. Thanks to the trait you can swap dependencies using a setter externally. Or mock it easily as well.

@dereuromark

This comment has been minimized.

Member

dereuromark commented Feb 13, 2018

It seems this is a rather controversial topic.
How about a compromise: Moving forward, but not adding deprecationWarning() into TableRegistry::get().
This way, people do not have to use the non-static approach here, but core can use it, and so can anyone who wants to.
There shouldnt be any harm in keeping it around a while longer. We just have added a new convenience access.

@lorenzo

This comment has been minimized.

Member

lorenzo commented Feb 13, 2018

It's definitely going to be deprecation hell for people migrating to cake 3.6.

@robertpustulka can you change the readme for the ORM package so that it shows you now need to instantiate a table locator?

@dereuromark

This comment has been minimized.

Member

dereuromark commented Feb 13, 2018

Yeah, since it really just delegates it through, I vote for keeping both around.
No harm, no additional baggage really IMO.

@robertpustulka

This comment has been minimized.

Member

robertpustulka commented Feb 13, 2018

@lorenzo readme is updated in this PR as well: https://github.com/cakephp/cakephp/pull/10933/files#diff-be6e833da32246111b12773eea161b28

I'm leaning towards @dereuromark opinion on this. I could remove warnings but keep deprecation annotations.

Another idea is to keep static API as it is but update the core to never use TableRegistry static methods and update docs to show how to use locators instead (this is already done here more or less). This way we could keep the static methods for people that use them (IDEs wouldn't mark methods as deprecated) but encourage new people to use locators instead (by never showing that something like TableRegistry exists). The static API could be depracated in 4.x and completely removed in 5.x then.

@lorenzo lorenzo closed this Feb 13, 2018

@lorenzo

This comment has been minimized.

Member

lorenzo commented Feb 13, 2018

fat fingers, sorry

@lorenzo lorenzo reopened this Feb 13, 2018

@lorenzo

This comment has been minimized.

Member

lorenzo commented Feb 13, 2018

@robertpustulka I meant that I wanted it to show first instantiating new TableLocator and then explain you can add the trait to classes.

robertpustulka added some commits Feb 15, 2018

Merge remote-tracking branch 'upstream/3.next' into deprecate-table-r…
…egistry

# Conflicts:
#	src/Network/Session/DatabaseSession.php
#	src/ORM/TableRegistry.php
#	src/TestSuite/TestCase.php
#	tests/TestCase/Auth/BasicAuthenticateTest.php
#	tests/TestCase/Auth/FormAuthenticateTest.php
#	tests/TestCase/Controller/Component/AuthComponentTest.php
#	tests/TestCase/Database/Schema/TableTest.php
#	tests/TestCase/ORM/Association/BelongsToManyTest.php
#	tests/TestCase/ORM/Behavior/BehaviorRegressionTest.php
#	tests/TestCase/ORM/Behavior/TimestampBehaviorTest.php
#	tests/TestCase/ORM/Behavior/TranslateBehaviorTest.php
#	tests/TestCase/ORM/Behavior/TreeBehaviorTest.php
#	tests/TestCase/ORM/CompositeKeysTest.php
#	tests/TestCase/ORM/Locator/LocatorAwareTraitTest.php
#	tests/TestCase/ORM/MarshallerTest.php
#	tests/TestCase/ORM/TableTest.php
#	tests/TestCase/View/Form/EntityContextTest.php
#	tests/TestCase/View/Helper/FormHelperTest.php
@robertpustulka

This comment has been minimized.

Member

robertpustulka commented Feb 15, 2018

I fixed conflicts.

Also removed deprecation warnings for TableRegistry methods. I left annotations so it's clear that the TableRegistry use should be avoided.

{
if (!$this->name) {
list(, $class) = namespaceSplit(get_class($this));
$this->name = str_replace(['Shell', 'Task'], '', $class);
}
$this->_io = $io ?: new ConsoleIo();
$this->_tableLocator = $locator;

This comment has been minimized.

@saeideng

saeideng Feb 15, 2018

Member

looks is useless

This comment has been minimized.

@robertpustulka

robertpustulka Feb 15, 2018

Member

The locator will fallback to the global one if is set to null.

@robertpustulka

This comment has been minimized.

Member

robertpustulka commented Feb 19, 2018

Can this be merged now?

@lorenzo

This comment has been minimized.

Member

lorenzo commented Feb 19, 2018

Yes

@robertpustulka robertpustulka merged commit a52a2ac into cakephp:3.next Feb 20, 2018

2 of 4 checks passed

codecov/patch 78.94% of diff hit (target 92.12%)
Details
codecov/project 92.1% (-0.03%) compared to fdfd026
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
stickler-ci No lint errors found.

@robertpustulka robertpustulka deleted the robertpustulka:deprecate-table-registry branch Feb 20, 2018

robertpustulka added a commit to cakephp/docs that referenced this pull request Feb 20, 2018

robertpustulka added a commit to cakephp/docs that referenced this pull request Feb 20, 2018

@markstory

This comment has been minimized.

Member

markstory commented Feb 20, 2018

@robertpustulka Can you update the 3.6 migration guide for these changes?

@robertpustulka

This comment has been minimized.

Member

robertpustulka commented Feb 20, 2018

Is that ok? cakephp/docs@d5a4362

@markstory

This comment has been minimized.

Member

markstory commented Feb 20, 2018

Yes. We should also start thinking about how we'd update code examples for when TableRegistry::get() is eventually removed.

@robertpustulka

This comment has been minimized.

Member

robertpustulka commented Feb 20, 2018

Static get calls have already been removed from the code. All tests now use a trait for accessing the locator.

@ADmad

This comment has been minimized.

Member

ADmad commented Feb 20, 2018

@robertpustulka I think he means the code examples in the manual.

@davidyell

This comment has been minimized.

Contributor

davidyell commented Feb 21, 2018

Yes, I would like to see a complete code implementation example somewhere with this new api.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment