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

Replace Date Format String Literals With Existing Constants #2592

Merged
merged 5 commits into from
Sep 23, 2022
Merged

Replace Date Format String Literals With Existing Constants #2592

merged 5 commits into from
Sep 23, 2022

Conversation

Sdfendor
Copy link
Contributor

@Sdfendor Sdfendor commented Sep 12, 2022

Description (*)

#2474 inspired me to search for usages of string literals containing the datetime format 'Y-m-d H:i:s' that is defined already as constant multiple times.

Varien_Db_Adapter_Pdo_Mysql::TIMESTAMP_FORMAT
Varien_Date::DATETIME_PHP_FORMAT

Imo to have multiple constants with the same content is fine because the constants have different contexts. This PR replaces string literals in each case with one of those two constants. I selected by context foreach case. I ensured that the corresponding columns in database have the timestamp type.

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All automated tests passed successfully (all builds are green)

…f the existing constants (db vs non-db context).
# Conflicts:
#	app/code/core/Mage/Adminhtml/Block/Sitemap/Grid/Renderer/Time.php
@github-actions github-actions bot added Component: AdminNotification Relates to Mage_AdminNotification Component: Adminhtml Relates to Mage_Adminhtml Component: Core Relates to Mage_Core Component: Dataflow Relates to Mage_Dataflow Component: lib/Varien Relates to lib/Varien Component: lib/* Relates to lib/* Component: Sitemap Relates to Mage_Sitemap labels Sep 12, 2022
@Sdfendor Sdfendor changed the title Replace date format string literals Replace Date Format String Literals With Existing Constants Sep 12, 2022
@@ -128,7 +128,7 @@ public function add($severity, $title, $description, $url = '', $isInternal = tr
if (is_array($description)) {
$description = '<ul><li>' . implode('</li><li>', $description) . '</li></ul>';
}
$date = date('Y-m-d H:i:s');
$date = date(Varien_Db_Adapter_Pdo_Mysql::TIMESTAMP_FORMAT);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd use Varien_Db_Adapter_Pdo_Mysql ::DATETIME_FORMAT since the name is more similar to the meaning, and the constant is the same

Copy link
Contributor Author

@Sdfendor Sdfendor Sep 12, 2022

Choose a reason for hiding this comment

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

I thought about those two. You are completely right, there is the third constant (besides the one in Varien_Date) that also defines a format and it is the same as TIMESTAMP_FORMAT. The other one is currently unused if the IDE is to be believed.
image

I picked TIMESTAMP_FORMAT for one main reason: the datrabase columns I checked all have the type timestamp not the type datetime which both are present in MySQL, are similar but have distinguishing characteristics. I totally think that the format of both will never be different though.
In all cases I checked there has been no column with datetime type. For the specific case this is true as well image.

Copy link
Contributor

Choose a reason for hiding this comment

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

wow, they've a timestamp type but the php formatting is YMDIms?? auch

sreichel
sreichel previously approved these changes Sep 12, 2022
@fballiano
Copy link
Contributor

fixed conflict

Copy link
Contributor

@fballiano fballiano left a comment

Choose a reason for hiding this comment

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

I'll approve although this timestamp thing still makes me wonder

@sreichel sreichel merged commit 47c8177 into OpenMage:1.9.4.x Sep 23, 2022
@github-actions
Copy link
Contributor

Unit Test Results

1 files  ±0  1 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
7 runs  ±0  5 ✔️ ±0  2 💤 ±0  0 ❌ ±0 

Results for commit 47c8177. ± Comparison against base commit 7358174.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Adminhtml Relates to Mage_Adminhtml Component: AdminNotification Relates to Mage_AdminNotification Component: Core Relates to Mage_Core Component: Dataflow Relates to Mage_Dataflow Component: lib/Varien Relates to lib/Varien Component: lib/* Relates to lib/* Component: Sitemap Relates to Mage_Sitemap
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants