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

Fix syntax in lib/Zend/Locale/Data/es_419.xml #1901

Merged
merged 1 commit into from
Dec 22, 2021

Conversation

elidrissidev
Copy link
Member

@elidrissidev elidrissidev commented Dec 16, 2021

Fixes a syntax error introduced in #1896 as spotted by @luigifab

Related Pull Requests

#1896

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)
  • Add yourself to contributors list

Co-authored-by: luigifab <31816829+luigifab@users.noreply.github.com>
@Flyingmana
Copy link
Contributor

anyone knows if this is causing an actual error in any case?
I would like to add a check for it.

@elidrissidev
Copy link
Member Author

It's weird because I was only able to reproduce the original issue after changing the locale to Spanish (Colombia), but not Spanish (Argentina), and even the merged fix doesn't fix it for me, cleared caches and everything. Has anyone tested this fix or am I missing something...

@luigifab
Copy link
Contributor

For me, by changing products quantity in cart, it does not work without initial PR for:

  • es_CL Spanish Chile
  • es_CO Spanish Colombia
  • es_CR Spanish Costa Rica
  • es_PA Spanish Panama
  • es_PE Spanish Peru
  • es_VE Spanish Venezuela

@luigifab
Copy link
Contributor

With the initial fixed PR, and after removed /tmp/zend_cache---*, it works for all es locales.
I checked es_419.xml but not (yet) tested Zend/Locale/Data/*.xml files from zf1-future, the proposed solution is not present.

@luigifab
Copy link
Contributor

I using this .git/hooks/pre-commit hook to check syntax of php and phtml and xml files for git commit:

#!/usr/bin/env bash
# sudo aptitude install libxml2-utils
 
PROJECT=$(php -r "echo dirname(dirname(dirname(realpath('$0'))));")
STAGED_FILES_CMD=$(git diff --cached --name-only --diff-filter=ACMR HEAD | grep '\.php\|\.phtml\|\.xml' )

# Determine if a file list is passed
if [ "$#" -eq 1 ]
then
	oIFS=$IFS
	IFS='
	'
	SFILES="$1"
	IFS=$oIFS
fi
SFILES=${SFILES:-$STAGED_FILES_CMD}

if [[ $SFILES ]]
then
	echo "Checking PHP/PHTML/XML..."
	for FILE in $SFILES
	do
		if [[ $FILE =~ ".xml" ]]
		then
			xmllint --noout $PROJECT/$FILE
		else
			php -l $PROJECT/$FILE
		fi

		if [ $? != 0 ]
		then
			echo "STOP"
			exit 1
		fi
		FILES="$FILES $PROJECT/$FILE"
	done
fi

exit $?

It's not perfect, and yes this is only local.

@elidrissidev
Copy link
Member Author

@luigifab does the additional > cause an error or not?

@luigifab
Copy link
Contributor

No there are no errors, even if I forgot to close a tag in any line of es_419.xml.
I suppose if the XML is invalid, it is ignored.

@addison74
Copy link
Contributor

Ideally all XML files should have tags that close. Otherwise it is not known what could come out of the concatenation of all the files. Most likely Magento ignores the error although we should have known about it. I will do a test in some XML files to see what happens. If anyone knows where to add some code to log the error in a particular XML file it would be perfect.

@Flyingmana
Copy link
Contributor

I created #1918 as followup to add a detection for this specific error, and xml syntax errors in general.

@Flyingmana Flyingmana merged commit 904baed into OpenMage:1.9.4.x Dec 22, 2021
@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 904baed. ± Comparison against base commit 8f8a20f.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: lib/* Relates to lib/*
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants