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

Mage_Log - DOC block update #767

Merged
merged 1 commit into from
May 5, 2020
Merged

Mage_Log - DOC block update #767

merged 1 commit into from
May 5, 2020

Conversation

sreichel
Copy link
Contributor

  • doc blocks added/fixed
  • PSR2 fixes (whitespaces, linebreaks, ...)

- doc blocks added/fixed
- PSR2 fixes (whitespaces, linebreaks, ...)
@sreichel sreichel added the Cleanup: DOC blocks Related to DOC block updates and fixes. label Jun 23, 2019
@@ -58,6 +58,9 @@ protected function _construct()
$this->_init('log/log');
}

/**
* @return float|int
Copy link
Contributor

Choose a reason for hiding this comment

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

why float? the config description is "Save Expired Items Days" and default value is "180"

Copy link
Contributor Author

@sreichel sreichel Apr 28, 2020

Choose a reason for hiding this comment

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

Mage::getStoreConfig(self::XML_LOG_CLEAN_DAYS) returns a string ... multiplied its float.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, so we need a casting to int here.
I propose we either skip this docbloc change, and open a new PR fixing the docblock and adding cast.

Copy link
Contributor

Choose a reason for hiding this comment

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

created an issue for this change #949

@@ -158,6 +187,9 @@ public function getUrl()
return $url;
}

/**
* @return mixed
Copy link
Contributor

Choose a reason for hiding this comment

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

why mixed? I wold expect rather int

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've used mixed only in a very few cases, where multiple return types are possble. Mixed/object are on todo list as mentioned in #784.

@@ -166,6 +198,9 @@ public function getFirstVisitAt()
return $this->getData('first_visit_at');
}

/**
* @return mixed
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See above.

@@ -58,6 +58,9 @@ protected function _construct()
$this->_init('log/log');
}

/**
* @return float|int
Copy link
Contributor

Choose a reason for hiding this comment

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

created an issue for this change #949

@tmotyl tmotyl merged commit 573d11b into OpenMage:1.9.4.x May 5, 2020
@tmotyl
Copy link
Contributor

tmotyl commented May 5, 2020

thanks @sreichel ! 🎉

@sreichel
Copy link
Contributor Author

sreichel commented May 6, 2020

thanks @sreichel ! tada

I am happily surprised :)

Didnt check ... if this is discussed in discord plmk.

@sreichel sreichel deleted the cleanup/log branch May 9, 2020 20:24
@sreichel sreichel added the Component: Log Relates to Mage_Log label Jun 5, 2020
@sreichel sreichel mentioned this pull request Jun 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cleanup: DOC blocks Related to DOC block updates and fixes. Component: Log Relates to Mage_Log
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants