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

Api improvements #1470

Merged
merged 4 commits into from
May 25, 2016
Merged

Conversation

connortechnology
Copy link
Member

This simplifies the configuration of the API by pulling the db connection info from zm.conf.

@josh4trunks
Copy link
Contributor

josh4trunks commented May 9, 2016

Any idea where I can put this code to split ZM_DB_HOST?
I realized I couldn't put it in the class, so maybe we need a function somewhere that can be called first?

        if (strpos(ZM_DB_HOST, ':')) {
                // Host variable may carry a port or socket.
                list($host, $portOrSocket) = explode(':', ZM_DB_HOST, 2);
                        if (is_numeric($portOrSocket)) {
                                $port = $portOrSocket;
                        } else {
                                $socket = $portOrSocket;
                        }
        } else {
                $host = ZM_DB_HOST;
        }

Here's an example, but I couldn't get it to work. maybe something needs to call the __construct function? I could get it to work mostly except for the list = explode part.
https://github.com/openshift/cakephp-example/blob/master/php/app/Config/database.php

I found a solution, I'll create a pull request based on your branch here.

@josh4trunks
Copy link
Contributor

PR for API is here.
connortechnology#1

This is ready for testing / merge.

@SteveGilvarry
Copy link
Member

@connortechnology checkout @josh4trunks changes and merge to your repo if you want them in this one, or let me know to let this one in as is.

Support user defined MySQL Port/Socket in API
@josh4trunks
Copy link
Contributor

josh4trunks commented May 13, 2016

Another PR to fix my earlier whitespace mistake
connortechnology#2

Ignore

@knight-of-ni
Copy link
Member

Seems to work with a default build (default zmuser, zmpass, sql port).

Note that this will require an API documentation update to specify it is no longer necessary to manually populate custom mysql credentials into the database.php file. Other than the README for Fedora & CentOS, I don't know where that documentation is.

@knight-of-ni knight-of-ni merged commit ceaacf9 into ZoneMinder:master May 25, 2016
@josh4trunks
Copy link
Contributor

=]
sweet, more automation

@connortechnology
Copy link
Member Author

I think this also means this file doesn't have to have cmake substitutions...

@tendonut
Copy link

tendonut commented Feb 9, 2017

You might want to update this doc to reflect this convenient change.
https://media.readthedocs.org/pdf/zoneminder/latest/zoneminder.pdf

It still suggests that the db user/pass is stored in api/app/Config/database.php.

@josh4trunks
Copy link
Contributor

How do we update the documentation? As of April 20, 2017 the PDF @tendonut linked is still outdated.

  • For the section "Changed Default DB User", point 2 can be deleted entirely.
  • Also in the section "New installs", in point 7, this statement can be deleted.
    Additionally, you must also edit /usr/share/zoneminder/www/api/app/Config/database.php in a similar manner on each ZoneMinder Server. Scroll down and change login and password to the values you created in the previous step.

@knight-of-ni
Copy link
Member

If you are referring to our readtehdocs documentation, there should be a link "Edit on Github" at the top right of the page, which will take you to proper file to edit under this folder:
https://github.com/ZoneMinder/ZoneMinder/tree/master/docs

@connortechnology connortechnology deleted the api_improvements branch April 21, 2017 17:35
@josh4trunks
Copy link
Contributor

@knnniggett sounds good, I made the changes I mentioned and will submit a PR.

I'm thinking I'll also add a note to zm.conf of the possible formats of ZM_DB_HOST, mentioned here.
#1498 (comment)

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.

5 participants