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

warning in > php 7.1 #4

Closed
proseLA opened this issue Feb 13, 2019 · 13 comments
Closed

warning in > php 7.1 #4

proseLA opened this issue Feb 13, 2019 · 13 comments

Comments

@proseLA
Copy link

proseLA commented Feb 13, 2019

if (strtolower(DATE_FORMAT) == 'm/d/y') {

cindy, this line now throws a warning. i believe the code should have quotes around it.

if (strtolower('DATE_FORMAT') == 'm/d/y') {

also on line 132.

@lat9
Copy link
Owner

lat9 commented Feb 13, 2019

What version of Zen Cart? That DATE_FORMAT constant is defined in /admin/includes/languages/english.php (or whichever language).

@lat9
Copy link
Owner

lat9 commented Feb 13, 2019

Let me guess ... Zen Cart 1.5.6, right? That's a result of the behavioral change made via this Zen Cart commit.

I'm wondering how many other plugins are going to be affected, since there's now no way of using those base language elements (like DATE_FORMAT or CHARSET) as the basis for plugin-specific definitions.

@proseLA
Copy link
Author

proseLA commented Feb 13, 2019

ok. i probably should have looked at this closer.

u r correct. zc 1.5.6

i will revert and look at that PR. hard to imagine the load order changing this way, but somehow not surprised.

ill confirm later today.

thanks.

@lat9
Copy link
Owner

lat9 commented Feb 13, 2019

I'll await your assessment, @proseLA.

@proseLA
Copy link
Author

proseLA commented Feb 13, 2019

@lat9 you are 100% correct. the changing of the load order is the problem.

it seems if we were to define DATE_FORMAT in the language file here, no error gets thrown when the language gets loaded later. to be clear, in:

ADMIN/includes/languages/english/stats_sales_report.php

(or whatever language file one uses...)

we would need to add:
define('DATE_FORMAT', 'm/d/Y');

(or whatever format one wants to use...)

then it looks like ZC will not log an error when the define happens again.

@lat9
Copy link
Owner

lat9 commented Feb 14, 2019

Yes, but the whole point was to re-use the base language's DATE_FORMAT so that a store normally doesn't have to define that value multiple times (and possibly out-of-sync with that main language file).

@proseLA
Copy link
Author

proseLA commented Feb 14, 2019

@lat9 on this issue, i can see both sides of the discussion. i do not feel strongly either way on it, and even if i did, i no longer get much joy in my participation/contributions on the project. i suppose an argument can be made that it is a defines file, and as such should not include any logic. given all of that, i think the choices are:

  • make an argument to revert the load order on the ZC project.
  • add the define as i indicated in my comment above.
  • make a new argument to include the logic and defines in the ZC language file on the ZC project.
  • move the logic for these defines out of the language files and into the code, perhaps into a function.

i would think the last option probably makes the most sense to me. but i can imagine you may have additional ideas or feel differently about the choice.

if i can help you out in any way, please let me know. best.

@lat9
Copy link
Owner

lat9 commented Feb 14, 2019

@proseLA, I'm thinking to go with bullet-point 4 for the change ... as you indicated. It just bugs the heck out of me that the change is needed at all.

lat9 added a commit that referenced this issue Feb 15, 2019
@lat9
Copy link
Owner

lat9 commented Feb 15, 2019

Actually, I chose to move those date-format settings into the 'main-line' of the report itself. Let me know if that works for you!

@lat9 lat9 added this to the v3.2.1 milestone Feb 15, 2019
@proseLA
Copy link
Author

proseLA commented Feb 15, 2019

@lat9, first off, these upgrades (v155 to v156) should NOT be this hard, oy! i have done far too much customizations over the years.

i was actually looking at this yesterday, and it seemed to make no sense why there were 4 separate constants. they are all the same. i tried playing around to get it to work, but alas too late and too much stuff going on.

this looks like it will work. i will give it a try and report back post haste. i will also make another attempt at using 1 constant. unless you can enlighten me as to why 4 might be required. best.

@lat9
Copy link
Owner

lat9 commented Feb 15, 2019

@proseLA, I kept those common constants so that I didn't need to make further updates. Until I get around to totally restructuring this report, I'm hesitant to make additional changes.

@proseLA
Copy link
Author

proseLA commented Feb 15, 2019

OK. this worked fine.

in addition, i think we can get by with 1 constant. but frankly i'm with you, lets not restructure the whole thing....

i have also noticed a peculiar little thing. if one wanted to look at info on a weekly basis for last month, the week starts on the day that the 1st of the month is. so the first week is always the 1st to the 7th; irregardless if the first is a tuesday or a friday, etc.

manipulating dates is always tricky.

but i'm totally comfortable with it the way that you currently have it. it works... clients have not complained about the week thing.... let's move on.

thanks again for all your work!

@proseLA proseLA closed this as completed Feb 15, 2019
@lat9
Copy link
Owner

lat9 commented Feb 15, 2019

Re-opening. FWIW, I keep issues open until they've been released!

@lat9 lat9 reopened this Feb 15, 2019
@lat9 lat9 closed this as completed Feb 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants