Funkcja do sprawdzania treści z formularza Opcje

[andrzejt17]

Proszę was o ocene skryptu pod kątem przydatności i funkcjonalności oraz proszę o wskazówki :) Czyli co zmienić, czego nie używać itp :)

[PHP] pobierz, plaintext 
<?php
// (kod)
 
function checkString($name, $value, $length, $type='all') {
	function showAlert($msg) {
		print ' <meta http-equiv="Content-Type" content="text/html; charset=utf-8">
				<script type="text/javascript">
					alert("'.$msg.'");
				</script>';
	}
 
	$value = strtolower(makeSafe($value));
	$charsToReplace = array('ą', 'ż', 'ź', 'ś', 'ę', 'Ć', 'ń', 'ł', 'ó', 'Ą', 'Ż', 'Ź', 'Ś', 'Ę', 'Ć', 'Ń', 'Ł', 'Ó');
	$valueToCount = str_replace($charsToReplace, 'x', $value);
	$length = explode('-', $length);
	if(strlen($length[0]) > 0 && strlen($length[1]) > 0) {
		$min = $length[0];
		$max = $length[1];
	} else {
		echo 'checkString length arg failed';
		exit;
	}
	if(strlen($valueToCount) == 0) {
		showAlert('Pole \''.$name.'\' jest obowiązkowe, nie może być puste.');
		return false;
	} elseif(strlen($valueToCount) <= $min) {
		showAlert('Liczba znaków w polu \''.$name.'\' jest za mała. Wymagana minimalna liczba znaków: '.$min.'.');
		return false;
	} elseif(strlen($valueToCount) >= $max) {
		showAlert('Liczba znaków w polu \''.$name.'\' jest za duża. Wymagana maksymalna liczba znaków: '.$max.'.');
		return false;
	}
 
	if($type == 'n') {
		if(!preg_match ('/^[0-9]+$/', $value)) {
			showAlert('Zawartość pola \''.$name.'\' musi być liczbą.');
			return false;
		}
	} elseif($type == 'tl') {
		if(!preg_match ('/^[0-9A-Za-z._-]+$/', $value)) {
			showAlert('Zawartość pola \''.$name.'\' może zawierać cyfry, małe i duże litery bez polskich znaków diakrytycznych oraz znaki specjalne: (._-)');
			return false;
		} 
	} elseif($type == 'm') {
		if(!preg_match ('/^([0-9a-z]([-.\w]*[0-9a-z])*@(([0-9a-z])+([-\w]*[0-9a-z])*\.)+[a-z]{2,6})$/i', $value)) {
			showAlert('Podany adres email jest nieprawidlowy.');
			return false;
		}
	}
 
	return true;	
}
 
// (kod)
?>
[PHP] pobierz, plaintext 

Do tego funcja makeSafe();

[PHP] pobierz, plaintext 
<?php
// (kod)
 
function makeSafe($string) {
	if(get_magic_quotes_gpc()) {
		$string = stripslashes($string);
	}
	$string = mysql_real_escape_string(trim($string));
	$string = htmlspecialchars($string);
	$string = str_replace("\r\n", "", $string);
 
	return $string;
}
 
 
// (kod)
?>
[PHP] pobierz, plaintext 

Wywyłanie:

[PHP] pobierz, plaintext 
<?php
// (kod)
// sprawdzenie testowo jednego pola z formularza
 
$imieUsera = makeSafe($_POST['imie']);
$nazwiskoUsera = makeSafe($_POST['nazwisko']);
 
if(checkString('Imie', $imieUsera, '3-10', $type='tl') && checkString('Nazwisko', $nazwiskoUsera, '3-10', $type='tl')) {
	echo 'Wszystko ok';
} else {
	echo 'Coś lipa';
}
 
// (kod)
?>
[PHP] pobierz, plaintext 

Ten post edytował andrzejt17 13.05.2011, 17:11:18

[Zyx]

Czego nie używać? Na pewno nie kodu powyżej. Twój kod przypomina serię z karabinu maszynowego puszczoną na oślep w tłum, przy czym jest duża szansa, że w tłumie nikt nie zostanie nawet ranny, a zginie od kuli jakaś staruszka 10 ulic dalej. Umiesz używać karabinu, ale nie umiesz nim jeszcze strzelić tam, gdzie chcesz. Musisz się nauczyć, że nie wolno wrzucać wszystkiego do jednego worka, bo to gwarantuje kłopoty. A u Ciebie w tym momencie funkcja robi wszystko:

- sprawdza długość tekstu
- sprawdza poprawność znaków
- sprawdza e-mail (!)
- wypisuje błędy
- generuje kod HTML

jeszcze tylko brakuje, by pranie robiła przy okazji. Bardzo nieumiejętnie posługujesz się dostępnymi w PHP funkcjami. Znajomość rozszerzeń i możliwości języka leży na całej linii:

[PHP] pobierz, plaintext 
$value = strtolower(makeSafe($value));
$charsToReplace = array('ą', 'ż', 'ź', 'ś', 'ę', 'Ć', 'ń', 'ł', 'ó', 'Ą', 'Ż', 'Ź', 'Ś', 'Ę', 'Ć', 'Ń', 'Ł', 'Ó');
$valueToCount = str_replace($charsToReplace, 'x', $value);
[PHP] pobierz, plaintext 

ŹLE - masz mb_strlen() (już pomijam fakt, że w tych trzech linijkach jest błąd, ale o tym dalej)

[PHP] pobierz, plaintext 
$length = explode('-', $length);
if(strlen($length[0]) > 0 && strlen($length[1]) > 0) {
[PHP] pobierz, plaintext 

ŹLE - masz tablice!

[PHP] pobierz, plaintext 
if(!preg_match ('/^[0-9]+$/', $value)) {
	showAlert('Zawartość pola \''.$name.'\' musi być liczbą.');
	return false;
}
[PHP] pobierz, plaintext 

ŹLE - masz ctype_digit()

[PHP] pobierz, plaintext 
if(!preg_match ('/^[0-9A-Za-z._-]+$/', $value)) {
[PHP] pobierz, plaintext 

ŹLE - masz ctype_alnum()

[PHP] pobierz, plaintext 
if(!preg_match ('/^([0-9a-z]([-.\w]*[0-9a-z])*@(([0-9a-z])+([-\w]*[0-9a-z])*\.)+[a-z]{2,6})$/i', $value)) {
[PHP] pobierz, plaintext 

ŹLE - masz filter_var()

[PHP] pobierz, plaintext 
function makeSafe($string) {
	if(get_magic_quotes_gpc()) {
		$string = stripslashes($string);
	}
	$string = mysql_real_escape_string(trim($string));
[PHP] pobierz, plaintext 

ŹLE - primo, dobrze napisany skrypt powinien zakładać, że dane wejściowe nie są wyescape'owane i escape'ować je TYLKO W RAZIE POTRZEBY. Ponadto wytłumacz mi, jaka jest logika robienia najpierw stripslashes() tylko po to, by chwilę później zrobić mysql_real_escape_string()? I wrócę teraz jeszcze do wspomnianego błędu: nie myślisz podczas kodowania. Spójrz na przykład: przed wywołaniem checkString() przepuszczasz wartość przez makeSafe(). A co robisz w checkString()? Przepuszczasz ją przez tę funkcję ponownie.

[thek]

Ogólnie z tym co napisałeś Zyx się zgadzam, ale doczepię się szczegółów:
1) ctype_alnum walnie true dla a-z0-9, ale w regexpie są jeszcze _.-, tak więc ctype_alnum tutaj nie może być użyty.
2) filter_var do sprawdzania maila, z tego co gdzieś w necie wyczytałem, jest zawodną funkcą i nie należy mu zbytnio ufać. Stąd wyrażenie regularne jest pewniejsze niż on i nawet w frameworkach jest to faworyzowane rozwiązanie. Nie wierzyłem do czasu, gdy mi także się zdarzyło, że wywalił false na poprawnym mailu i od tamtego czasu zarzuciłem używanie tej funkcji na rzecz regexp.
3) rozumiem sprawdzanie gpc z racji faktu, iż nigdy na początku nie wiemy, na jaki hosting się trafi i co ma w konfigu włączone a co nie. Jeśli z jakiejś głupiej przyczyny magic_quotes są włączone, to podczas obróbki i ponownego wkładania danych do formularza może zakończyć się niemiłymi niespodziankami. Stąd rozumiem czemu piszesz o tym, że dane powinny być nie escape'owane. Inna sprawa, że magic_quotes używają do escape'owania addslashes jeśli dobrze pamiętam. A ta funkcja jest dziurawa (IMG:style_emoticons/default/wink.gif) "Zastąpienie" jej poprzez mysql_real_escape_string jest pewniejsza.

Ogólnie jednak się zgodzę, że jest to pomieszanie z poplątaniem. Zwłaszcza patrząc na walenie mety w print co jest, delikatnie mówiąc, chybione.

[andrzejt17]

Panowie, dziękuję za wskazówki (IMG:style_emoticons/default/smile.gif) Jednak nasuwa mi się kilka pytań.

Do Zyx:

1. "Musisz się nauczyć, że nie wolno wrzucać wszystkiego do jednego worka, bo to gwarantuje kłopoty." Zapewne chodzi o funkcję checkString(). Czy jest sens robienia osobnej funkcji dla sprawdzania każdego typu danych? Np. checkNum(), checkMail() itd? Po to właśnie jest jeden z argumentów w checkString() by ustalić pod jakim kątem ma sprawdzać naszego stringa. Funkcja do tego celu jest mi niezbędna bo moja strona przewiduje kilka formularzy i uciążliwe byłoby pisanie tego samego w kilku miejscach. Jeśli będą takie możliwości w php to napiszę funkcję od prania ;D

2. O mb_strlen() nawet nie słyszałem (IMG:style_emoticons/default/haha.gif) Zakładam, że policzy mi ona cyferki (IMG:style_emoticons/default/wink.gif) Pytanie tylko czy musze podawać argument z kodowaniem znaków i jeśli tak to jaki będzie dla utf-8? ut8? (IMG:style_emoticons/default/snitch.gif)

3. "ŹLE - masz tablice!" Chodzi o to, że gdy nie będzie nic przed '-' albo po to wtedy będę miał tylko jedną tablice? Wtedy można tylko sprawdzić, czy wybrana tablica istnieje a nie sprawdzać czy jej wartość jest większa od 0, dobrze rozumuję?

4. ctype_digit(), po tym co napisał thek jednak zostanę przy swoim rozwiązaniu.

5. ctype_alnum(), chodzi mi też o znaki" _.- które mogą być użyte np. w loginie.

6. filter_var(), również zostanę przy swoim rozwiązaniu (thek).

7. Escapować tylko w razie potrzeby? Tzn wtedy, kiedy np. chce dodać wartość do bazy? Fakt, z tym escapowaniem przesadziłem dlatego uszczupliłem funkcję makeSafe() i używał jej będę tylko raz, przy pobieraniu treści z POST'a. stripslashes() poszedł w kosz.


Do: thek

Funkcja sprawdzania formularza jest wywoływana przed <html> w dokumencie co oznacza, że polskie znaki zwracane w print przez funkcję zostaną wyświetlone z krzakami (meta w head). Dlatego ta meta tam siedzi bo już stamtąd zwracam komunikat userowi, nie mam pomysłu na coś lepszego (nie chce tego robić echem).


== EDIT ==
Acha, jeszcze jedno (IMG:style_emoticons/default/smile.gif) Jak by ktoś pytał, showAlert() jest wewnątrz chechString() dlatego, że tylko tam mi ona potrzebna. Do zwracania reszty komunikatów używam innej funkcji:

[PHP] pobierz, plaintext 
<?php
 
function jsAlert($errorCode, $url='0') {
	switch($errorCode) {	
		case 100:
			$message = 'Ta operacja wymaga podniesienia upranień. Zaloguj się.';
			break;
		case 101:
			$message = 'Nie masz uprawnień do tej części serwisu. Zaloguj się.';
			break;
		// dalsze kody błędów
	}
 
	if($url == '0') {
		print ' <script type="text/javascript">
					alert("'.$message.'");
				</script>';	
	} else {
		print ' <script type="text/javascript">
					if(confirm("'.$message.'")) {
						window.location = "'.$url.'"
					} else {
						window.location = "'.$url.'"
					}
				</script> ';	
	}
}
 
?>
[PHP] pobierz, plaintext 

Kod pewnie też kaszana ale nie mam lepszego pomysłu na zwracanie komunikatów. Ten mi wiernie służył i nigdy nie zawiódł (IMG:style_emoticons/default/wink.gif)

== EDIT2 ==
Jak się właśnie okazało kod nie spełnia swojej roli tak jak bym tego chciał. Okazuje się, że:

[PHP] pobierz, plaintext 
if(checkString('nazwa1', 'wartosc', '2-10') && checkString('nazwa2', $wartosc2, '2-10')) {
// akcja
}
[PHP] pobierz, plaintext 

nie przynosi żądanego rezultatu. Jeśli w if mam tylko jedno wywołanie to działa jak należy, jeśli dwa czy więcej to checkString() nie robi nic (pusta strona).

Ten post edytował andrzejt17 13.05.2011, 23:47:50

[thek]

Dziwisz się? IF to operacja logiczna i wystarczy jeden false a cały warunek idzie w piach, bo używasz && czyli I logicznego. A zobacz kiedy to jest prawdziwe... Tylko gdy wszystkie warunki są prawdziwe. Jedna pierdółka na false i wszystko leży.

Dorzućmy odpowiedzi Ci... Czy jest sens stosować oddzielnie funkcje? Tak! Ponieważ wtedy masz je bardziej uniwersalne. Zamiast potem pisać pierdylion funkcji lub wywoływać jedną z różnymi, nie zawsze sensownymi parametrami, posługujesz się kilkoma w konkretnych sytuacjach. I tak musisz przejść przez nią całą i sprawdzić X warunków IF i jako parametr w wywołaniu dla tych miejsc które opuszczasz ustawić "tego nie rób". Innymi słowy: mniejsza funkcja -> mniej roboty. A czy jest sens robić jeden kombajn i pamiętać co jaki parametr znaczy czy prościej zrobić mniejsze i wyspecjalizowane funkcje, które nie tylko w tym jednym miejscu wykorzystasz, ale w dowolnym fragmencie?

ctype_digit akurat działa dobrze, poza tym nic o niej nie pisałem (IMG:style_emoticons/default/smile.gif)

Jeśli printujesz coś PRZED html, to chyba zupełnie nie rozumiesz, że tak nie powinno się robić. dokument html powinien się znacznikiem html zaczynać czy też specyfikacją DTD i nic nie ma prawa przed nim być wypisane. To zwyczajny błąd. To co chcesz osiągnąć, czyli wyświetlenie komunikatu błędu wraz z przekierowaniem na określoną stronę, rozwiązuje się zupełnie inaczej... Samą walidację także podejrzyj w nieco innych stronach, bardziej profesjonalnych - jakieś darmowe cms choćby.

[andrzejt17]

Czyli jestem zmuszony robić coś w tym stylu:

[PHP] pobierz, plaintext 
if(strlen($wartosc) < 3) {
	echo 'za malo znakow';
	exit;
} elseif(strlen($wartosc) > 15) {
	echo 'za duzo znakow';
	exit;
} elseif(!ctype_digit($wartosc)) {
	echo 'nie jest liczba';
	exit;
}
[PHP] pobierz, plaintext 

A chciałem ciachnąć fały formularz w jednym if'ie (IMG:style_emoticons/default/wink.gif) W sumie tak to nawet lepiej, przejrzyściej (IMG:style_emoticons/default/wink.gif)

Przekonałeś mnie z tymi osobnymi funkcjami. Faktycznie lepiej jest napisać funkcję do jednej konkretnej rzeczy a nie jedną do wszystkiego.

Z tym ctype_digit() faktycznie, nie pisałeś. Musiało mi się coś pomieszać, nie spałem tamtą noc więc zwyczajnie już nie ogarniałem (IMG:style_emoticons/default/wink.gif)

Jeśli coś printuje, jakiś kod html, no dynamiczny formularz czy jakiś inny kod to oczywiście funkcję, która to robi wywołuję już po znaczniku <body> wiec wszystko jest cacy zachowane. Inaczej to wygląda w wypadku zwracania komunikatu userowi przez alert w js. Jakimś cudem to działa na moim hostingu, on jakby za mnie wcześniej dopisuje html, body i całą resztę to nawet jak je pousuwam ze swojej strine to i tak wszystko hula (hosting dołącza reklamy więc pewnie dlatego).

Tak czy inaczej będę musiał pomyśleć nad jakąś metodą zwracania komunikatów userowi. Chce to zrobić w alercie w js bo jest najprościej. Może jak bd printował alerta to może tam dorzucę znaczniki niezbędne w html byle tylko to wyświetlić. (IMG:style_emoticons/default/wink.gif)

[thek]

Co do długości... Nie pomyślałeś o metodzie po prostu walidującej długość jako zakres min, max?
valid_length( $string, $min = false, $max = false)

Jeśli będzie błąd, zwróć komunikat/y błędu, jeśli OK zwróć TRUE. Przy sprawdzeniu zrób === TRUE (IMG:style_emoticons/default/smile.gif) To tylko jedna z implementacji.

[PHP] pobierz, plaintext 
function valid_length( $string, $min = false, $max = false) {
    $error = array();
    $valid_min = false;
    if($min && ctype_digit($min) && $min > 0 ) {
        $valid_min = true;
        if( mb_strlen($string ) < $min ) {
            $error[] = 'Ciąg za krótki. Wymagane '.$min.' znaków';
        }
    }
    if($max && ctype_digit($max) && $max > 0 ) {
        if( mb_strlen($string ) < $max ) {
            $error[] = 'Ciąg za długi. Maksymalnie '.$max.' znaków';
        }
        if($min == false || $valid_min ) {
            if( $max - (int)$min < 0) {
                $error[] = "Minimum jest więkse niż maximum.";
            }
        }
    }
    if( count( $error ) > 0 ) {
        return $error;
    } else {
        return true;
    }
}
[PHP] pobierz, plaintext 

Oczywiście pewne rzeczy można rozwiązać ładniej poprzez choćby rzucenie wyjątku (IMG:style_emoticons/default/wink.gif) Ale na Twoim etapie znajomości php tylko sobie za mocno byś skomplikował kod.

[Zyx]

Funkcje sprawdzające poprawność danych to dopiero początek, a nie powinny one niczego wyświetlać z prostego powodu: zaczniesz nowy projekt, skopiujesz kod i klops. Musisz przepisywać swoje funkcje, by je dopasować do nowej szaty graficznej, nowych wymagań i na pewno przy tym przepisywaniu zrobisz gdzieś błąd. Kod po to dzieli się na mniejsze, wyspecjalizowane w jednym zadaniu kawałki, by można ich było używać wielokrotnie w różnych sytuacjach.

[andrzejt17]

Używam funkcji jsAlert() W argumencie podaje kod błędu a 2 drugim podaje nazwe pliku na który ma mnie przekierować po kliknięciu 'ok' przez użytkownika. Jeśli więc zwracam komunikat to zamiast tego echo co tam pisałem daje np jsAlert(100, 'index.php'); i jest git (IMG:style_emoticons/default/smile.gif) Jakoś nie czuje potrzeby napisania strony dla błędów, takie komunikaciki z powodzeniem mi wystarczają (IMG:style_emoticons/default/smile.gif)

Wezmę sobie do serca radę z funkcjami, że lepiej zrobić każdą funkcję do określonego zadania (IMG:style_emoticons/default/smile.gif)

Generalnie walidację formularza zrobię łącząc kawałek swojego sposobu i kawałek thek'a (IMG:style_emoticons/default/smile.gif)

Panowie, dziękuję za odpowiedzi.