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

Changes required for accessibility page #325

Merged
merged 1 commit into from Mar 4, 2024

Conversation

scottcwilson
Copy link

@scottcwilson scottcwilson commented Mar 3, 2024

Checks if FILENAME_ACCESSIBILITY is defined for backwards compatibility and compatibility with older Zen Cart versions that have used the accessibility page plugin. Tested on 2.0.0-b1 with plugin installed (the build was done before Zen Cart PR zencart/zencart#6269 was merged.)

@lat9
Copy link
Owner

lat9 commented Mar 3, 2024

I'll grant that I've not followed the incorporation into zc200, but shouldn't there be a means for a site to "not include" that page in the sitemap/information-sidebox even if the page exists?

@scottcwilson
Copy link
Author

You can hide it with a flag that exists in the new site_specific storefront file. I have added a check if they wish to add this setting to their copy of the file.

Copy link
Owner

@lat9 lat9 left a comment

Choose a reason for hiding this comment

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

Use !empty() in both places.

@scottcwilson
Copy link
Author

Use !empty() in both places.

!empty won't work in older versions of Zen Cart because this flag is only set in the newest version of includes/init_includes/init_common_elements.php. That's why I did it this way.

@drbyte
Copy link

drbyte commented Mar 4, 2024

empty($var) === true in the following conditions:

  • $var not defined
  • $var === 0
  • $var == 0
  • $var == '0'
  • $var == ''
  • $var === false
  • $var === null

Basically: falsey, null, empty, blank, missing

That's why we use it in a lot of places. Especially for backward compatibility.
(There are of course some cases where we 'do' need an isset() because we want to know that, but in the case of a flag that's only used for an on/off switch, it's moot.)

Doesn't work on "undefined constants" though. Those will throw an error about undefined constant before passing thru to empty().

@scottcwilson
Copy link
Author

So is this ready to merge then?

@dennisns7d
Copy link

It appears to me that !isset($flag_show_accessibility_sidebox_link) || $flag_show_accessibility_sidebox_link == 0) reverses the desired boolean value when $flag_show_accessibility_sidebox is defined.
Shouldn't it be the same as the expression in includes/init_includes/init_common_elements.php - (isset($flag_show_accessibility_sidebox_link)) ? (bool)$flag_show_accessibility_sidebox_link : true?

@scottcwilson
Copy link
Author

@dennisns7d is correct in his first statement. Thank you!

@lat9
Copy link
Owner

lat9 commented Mar 4, 2024

I'm still not liking this and still believe that !empty() is the right approach. If the template's running on zc200, then the filename is defined and init_common_elements.php has defaulted the value to (bool)true if not previously defined, so the sidebox link will be displayed.

If running on zc157/zc158, then the site has to take additional actions (i.e. both defining that FILENAME_ACCESSIBILITY, setting the "soft" setting, copying over the pages/accessibility files as well as the associated language file.

@scottcwilson
Copy link
Author

Done. Please merge.

@lat9
Copy link
Owner

lat9 commented Mar 4, 2024

Addresses issue #327.

@lat9 lat9 merged commit 5a818f4 into lat9:v300 Mar 4, 2024
@scottcwilson scottcwilson deleted the accessibility branch March 4, 2024 23:24
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.

None yet

4 participants