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

[WIP] Update config for PerformanceInfo pluging #7318

Closed
wants to merge 1 commit into from
Closed

[WIP] Update config for PerformanceInfo pluging #7318

wants to merge 1 commit into from

Conversation

ThaDafinser
Copy link
Contributor

https://github.com/ThaDafinser/Piwik-PerformanceInfo

Need another public method for the global.ini path.

@mattab
Copy link
Member

mattab commented Mar 1, 2015

Hi @ThaDafinser can you explain what you are trying to achieve?

I think currently you can already load the local values by doing Config::getInstance()->Section so I'm not sure if this PR is needed?

@mattab mattab added the Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. label Mar 1, 2015
@mnapoli
Copy link
Contributor

mnapoli commented Mar 1, 2015

That might conflict with the refactoring in #7312, @diosmosis might have some more info/alternative solution?

The method is not tagged @api, which means we can refactor it later and break your plugin. If you add @api that means we have to stay backward compatible on this, so I think there should be a compelling reason to do so.

@diosmosis
Copy link
Member

To get from local config, you should just access the section directly. Sections in the local config completely override default settings in global/common/etc., so the method isn't needed.

@mattab mattab closed this Mar 3, 2015
@ThaDafinser
Copy link
Contributor Author

@diosmosis i would like to show the user where the value come from, see here:
ThaDafinser/Piwik-PerformanceInfo#2

If i just use the last value it might be set in the local config or not. (e.g. Global and local could be the same)

@ThaDafinser
Copy link
Contributor Author

@diosmosis
Copy link
Member

Ah, ok, I see what your use case is now. The method in the pull request is probably the best solution to this, and it can be easily supported/maintained by the changes in #7312. @mattab Feel free to merge.

@mattab mattab reopened this Mar 4, 2015
@mattab
Copy link
Member

mattab commented Mar 4, 2015

before merging we would need to

  • add a test for it,
  • tag method with @api (and possibly others that you need in your plugin) so we don't break them in the future

@mattab mattab added this to the Short term milestone Mar 20, 2015
@tsteur
Copy link
Member

tsteur commented Apr 7, 2015

I recently needed this method nearly as well. Would be nice to merge this after adding an @api as Matt suggested. Maybe you can also add a basic test to https://github.com/piwik/piwik/blob/2.13.0-b1/tests/PHPUnit/Unit/ConfigTest.php#L45

BTW: I noticed this is in the Unit test group when it is actually an integration test as it access the file system etc but we can leave it there for now.

@ThaDafinser
Copy link
Contributor Author

see the new PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants