Skip to content
This repository has been archived by the owner on May 14, 2018. It is now read-only.

Allow for custom table and adapter names #105

Closed
wants to merge 1 commit into from

Conversation

Freeaqingme
Copy link
Contributor

No description provided.

public function getTableName()
{
if (!$this->tableName) {
$this->setTableName(static::DEFAULT_TABLE_NAME);
Copy link
Contributor

Choose a reason for hiding this comment

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

Set this on the property instead and remove the getter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why do you want to remove the getter? The way I did it was in line with other modules like ZFC.

Copy link
Contributor

Choose a reason for hiding this comment

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

The getter here adds unnecessary logic that simply handles the default value of the property. This class should actually be more like an immutable service, so setters/getters should probably completely be removed

@Ocramius
Copy link
Contributor

Ocramius commented Mar 5, 2013

Can you please also add tests for the factory at least? Testing the provider itself is probably too messy

@Freeaqingme
Copy link
Contributor Author

Oops, kinda missed the fact that you had tests. Will provide these asap.

@Ocramius
Copy link
Contributor

Ocramius commented Mar 6, 2013

Please also rebase (I fixed the travis runner in the mean time)

@Ocramius
Copy link
Contributor

Cannot merge without rebase anymore (FYI)

@Ocramius
Copy link
Contributor

Closing - please eventually open a new PR

@Ocramius Ocramius closed this Apr 10, 2013
@ghost
Copy link

ghost commented Apr 30, 2013

If this feature is still not merged we can't change the table name via the config file ?

Should we open a new PR ?

@Ocramius
Copy link
Contributor

@Angusfr yes please

@ghost
Copy link

ghost commented Apr 30, 2013

k i'm gonna check how to do this.

My first time :')

@Ocramius
Copy link
Contributor

@Angusfr give it a try! And don't worry about messing it up :)

@ghost
Copy link

ghost commented Apr 30, 2013

Should i use the code previously submitted here ?

@Ocramius
Copy link
Contributor

@Angusfr give it a try from scratch

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants