search

Ecodev / newsletter

TYPO3 extension to send newsletter
https://extensions.typo3.org/extension/newsletter/
25 stars 26 forks source link

Siwa pparzer patch 1 #96

Closed siwa-pparzer closed 8 years ago

siwa-pparzer commented 8 years ago

respecting the storage page if constant module.tx_newsletter.persistence.storagePid is set

PowerKiKi commented 8 years ago

This PR does not respect coding style and thus fails on Travis. Please use the following to check and fix issues:

npm install -g gulp
npm install
gulp
siwa-pparzer commented 8 years ago

is this PR ok now?

PowerKiKi commented 8 years ago

I believe hasStoragePid should be private and a boolean, not an integer, and phpDocumented as such.

But most importantly the repository should be self-aware of its own configuration and not dependent on who is using it. So instead of "check settings in controller, configure repository and then use it", rather do "self-configure repository upon initialization". It seems that this answer should give us a clue on how to get settings from within the repository.

siwa-pparzer commented 8 years ago

the repository gets the storagePid now via objectManager the controller is the same now as in the master branch code checks seem good

PowerKiKi commented 8 years ago

Looks much better, thanks for the modifications. There is only that file header that should not have been indented.

The last thing we need is to document this new behavior in documentation. I guess in Documentation/Configuration/Index.rst would be fine.

And finally if you squash all your commit into one, then I'll merge.

siwa-pparzer commented 8 years ago

commits merged all checks passed

PowerKiKi commented 8 years ago

Merged in 499403d768ccd3b2796b7f47a53adbda3fec3453, thanks for your work !