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

3.0 cache log config changes #1534

Merged
merged 9 commits into from Aug 17, 2013

Conversation

markstory
Copy link
Member

After @lorenzo 's comments on my last pull request (#1517) I've added the ability to read config data back out of Cache and Log. In addition to that I've done a few other things.

  • Made it simpler to inject built objects into Cache/Log.
  • Made it simple to use a factory function to help with more complicated engine construction.
  • Renamed 'engine' to 'className'. This is more conformant to the rest of CakePHP. engine will continue working though for now.

I also removed Cache.disable from Configure. I've always disliked this configure value. It is now part of the Cache class so it doesn't need to interact with Configure making the check cheaper, simpler and Cache more self contained.

I think there is an opportunity to extract config() and drop() into a trait if there are a few more similar uses. With only two uses it doesn't make much sense right now. But if another class re-uses the same pattern it will be useful.

I spent some time and drafted up some ideas on how config methods on other classes could work as well. Any feedback on that is more than welcome.

Allow various types of configuration to be provided. The new variants
allow for simpler dependency injection and still afford configuration
files.

Reconfiguring an adapter is now not allowed, as the configuration
changes were never propagated to the adapter. This results in a silent
failure that is hard to detect.

Move/remove tests around to make sense with the new API's
Remove 'engine' from the settings in each engine. It provides no value
and is never referenced outside of tests.
When converting engine => className the old key should be removed.
Make Log::config() accept the same data as Cache.
Give it a set of named methods on Cache. This configure value always
felt out of place and fits much better with Cache's feature set.
@jippi
Copy link
Contributor

jippi commented Aug 17, 2013

👍

except: there seem to be a lack of those newlines we also talked about in the other PR

and

there isn't any way to change "_enabled" without calling a method, thus not allowing cake to be configured / bootstrap from say, and environment ini file ?


static::$_config = array_merge(static::$_config, $key);
if (isset(static::$_config[$key])) {
throw new Error\Exception(__d('cake_dev', 'Cannot reconfigure existing adapter "%s"', $key));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a great idea and for the same reasons I think Cache::set() should get the axe

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm glad I'm not the only person who hates Cache::set(). That method is all sorts of silly. I'm on board for removing it if others are. It adds a pile of complexity to Cache for almost no benefit other than making the calling code more complicated as well.

@lorenzo
Copy link
Member

lorenzo commented Aug 17, 2013

Good job mark! There is one failing test in travis that you might want to take a look to, something related to auto creating the tmp folders

@markstory
Copy link
Member Author

@jippi Would you ever setup a deployed app with caching off? I couldn't think of a useful scenario where you'd want to configure a deployed application with caching disabled.

@markstory
Copy link
Member Author

@lorenzo What did you think of the other examples/proposals in the wiki page?

@jippi
Copy link
Contributor

jippi commented Aug 17, 2013

@markstory I could do that, or have it disabled in dev and enable it in prod - its also just for consistency that you can configure everything in a simple manner without using php

@lorenzo
Copy link
Member

lorenzo commented Aug 17, 2013

@markstory I think it is heading a good direction, the only thing that i could not understand is what is the difference between Email::configTransport and Email::config

@jrbasso
Copy link
Member

jrbasso commented Aug 17, 2013

The config method from Log and Cache are almost identical. What do you think in make it a trait? Probably other classes will be able to use it too.

@markstory
Copy link
Member Author

@jippi What if Cache::enabled() took a parameter to set the state? Kind of like a generic get/set that could be populated with data from a config file?

<?php
Cache::enabled(Configure::read('App.cacheEnabled'));

@markstory
Copy link
Member Author

@jrbasso Yes they are almost identical. Once there is a third copy I'll extract it into a trait.

@lorenzo I thought it might be simpler code/API wise if transports and delivery profiles were separate. That way if your application had multiple delivery profiles that re-used the same transport you would be able to avoid duplicating the transport settings multiple times.

@lorenzo
Copy link
Member

lorenzo commented Aug 17, 2013

@markstory That's a cool idea, I always hated to configure the same transport for different profiles. I also like the Cache::enabled() thing better

@ADmad
Copy link
Member

ADmad commented Aug 17, 2013

@lorenzo In 2.x we also have the problem that delivery config bled into transport. Eg. Doing $CakeEmail->config(array('log' => true)) the log key ends up in transport config too.

@jippi
Copy link
Contributor

jippi commented Aug 17, 2013

@markstory could work, but would require php to configure the app, and not just a configure load of an ini file

@markstory
Copy link
Member Author

@lorenzo Yeah having to duplicate transport settings was painful. Also the problem @ADmad pointed out created ambiguity in what would happen when Email::config() was called.

@jippi The current cache and log config methods will each require at least 1 line of PHP code. For example if you were using Configure's features to load/use configuration files you would have to do something like:

<?php
// app/Config/bootstrap.php
Configure::config('default', new IniReader());
Configure::load('caching');
Configure::load('logging');

Log::config(Configure::read('Log'));
Cache::config(Configure::read('Log'));
Cache::enabled(Configure::read('Cache.enabled'));

The point of this and the last set of changes was to remove the magic, ability to get data out of sync and spooky action at a distance that not having to use any methods caused. I feel this code while 3 lines longer is far more transparent and consistent.

Use correct functions and fix failing tests on travis.ci. Directories
were being left behind after tests were complete as parent::tearDown()
restores debug which allows directory to be created in teardown. Shuffle
parent and clear() calls to prevent this.
@markstory
Copy link
Member Author

All non CS tests are now passing on this branch.

lorenzo added a commit that referenced this pull request Aug 17, 2013
@lorenzo lorenzo merged commit 53b3d6c into cakephp:3.0 Aug 17, 2013
@markstory markstory deleted the 3.0-cache-log-config-changes branch November 10, 2013 22:26
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

5 participants