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

Feature/redis multiple databases #571

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
7 participants
@korotovsky
Copy link
Contributor

commented Sep 25, 2013

No description provided.

@DavertMik

This comment has been minimized.

Copy link
Member

commented Sep 26, 2013

@judgedim @tiger-seo Any comments?

I think we can get implemented the same via module inheritance:

<?php
class Redis2Helper extends Redis {}
modues: [Redis, Redis2Helper]
config:
  Redis: ~
  Redis2Helper: ~
@judgedim

This comment has been minimized.

Copy link
Contributor

commented Sep 27, 2013

@DavertMik, I support your option with Redis2Helper.

@korotovsky

This comment has been minimized.

Copy link
Contributor Author

commented Sep 27, 2013

@judgedim Why should the extended module be in the same directory with helpers?

@tiger-seo

This comment has been minimized.

Copy link
Member

commented Oct 2, 2013

@korotovsky there is not much difference between modules and helpers ;)

@tiger-seo

This comment has been minimized.

Copy link
Member

commented Oct 2, 2013

btw, i would suggest SecondRedisHelper instead of redis to helper :-)

@tiger-seo

This comment has been minimized.

Copy link
Member

commented Oct 2, 2013

forgot to include example :)

<?php

namespace Codeception\Module;

/**
 * For more info please refer to documentation for Db module
 *
 */

class SecondDbHelper extends Db
{
    public static $includeInheritedActions = false;

    /**
     * Inserts SQL record into database
     *
     * ``` php
     * <?php
     * $I->haveInDatabase('users', array('name' => 'miles', 'email' => 'miles@davis.com'));
     * ?>
     * ```
     *
     * @param       $table
     * @param array $data
     */
    public function haveInSecondDatabase($table, array $data)
    {
        parent::haveInDatabase($table, $data);
    }

    public function seeInSecondDatabase($table, $criteria = array())
    {
        parent::seeInDatabase($table, $criteria);
    }

    public function dontSeeInSecondDatabase($table, $criteria = array())
    {
        parent::dontSeeInDatabase($table, $criteria);
    }

    public function grabFromSecondDatabase($table, $column, $criteria = array())
    {
        return parent::grabFromDatabase($table, $column, $criteria);
    }
}
@korotovsky

This comment has been minimized.

Copy link
Contributor Author

commented Oct 2, 2013

@tiger-seo My changes are not hard to apply.

@pgolomysov

This comment has been minimized.

Copy link

commented Oct 2, 2013

@lancedevnull

This comment has been minimized.

Copy link

commented Oct 2, 2013

yes, it's need )

@tiger-seo

This comment has been minimized.

Copy link
Member

commented Oct 2, 2013

to be honest, i don't like this implementation, because it's kinda transform this module from Redis to RedisPool, so that you will not be able to operate (insert,delete,read) with database without specifying its name

@tiger-seo

This comment has been minimized.

Copy link
Member

commented Oct 2, 2013

you brought friends :))))

@korotovsky

This comment has been minimized.

Copy link
Contributor Author

commented Oct 2, 2013

@tiger-seo

to be honest

it sounds better :)

transforms this module from Redis to RedisPool,

I think the copy-paste module Redis to RedisPool isn't a good idea too. And... original module doesn't provide insert, delete and read operations anyway.

@tiger-seo

This comment has been minimized.

Copy link
Member

commented Oct 2, 2013

original module doesn't provide insert, delete and read operations anyway

yeah, but we can add them, as soon as someone would need them

@korotovsky

This comment has been minimized.

Copy link
Contributor Author

commented Oct 2, 2013

@tiger-seo I need multiple databases! Add them :)

@tiger-seo

This comment has been minimized.

Copy link
Member

commented Oct 2, 2013

Michael and me have already gave you advice how to meet your requirements without changing core modules, and my opinion you've read before, but final decision is up to @DavertMik

@posohin

This comment has been minimized.

Copy link

commented Oct 2, 2013

@DavertMik

This comment has been minimized.

Copy link
Member

commented Oct 2, 2013

Ok. Basically, the problem is that we just can't introduce more and more and more functionality.
It was a good idea when project started, but today the more we insert into project the more rakes on support we get.

Sure, current Redis module is far from ideal. It lacks CRUD functionality, which we have in other storage modules.
But this is planned to get it there one day. Maybe :) Anyway, there is one shared principle of how storage modules work, and I'd rather not change it.

There is absolutely no need to insert everything you might ever need into core. With tools like Composer we can create third-party addons, and as you may see we already have extensions section on site. Sure, you ca create RedisPool module in separate repo, and use it in your project. If it is installed via Composer, this module will be autoloaded, and can be used in application with no problem.

@posohin @pgolomysov @lancedevnull @korotovsky
Sorry to close this, but why not to get this done in separate repo?
@tiger-seo has shown you the sample, and so you can do just the same for Redis module.
That would be good extension.

@DavertMik DavertMik closed this Oct 2, 2013

@posohin

This comment has been minimized.

Copy link

commented Oct 2, 2013

That's all right. It's good idea to get this done in separate repo. Maybe if it will be popular you can move it in main repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.