Skip to content

[Fix] Nicht die _GET-Variable überschreiben#3

Closed
darookee wants to merge 3 commits intoshopware5:masterfrom
darookee:remove-set-_GET-in-sArticle
Closed

[Fix] Nicht die _GET-Variable überschreiben#3
darookee wants to merge 3 commits intoshopware5:masterfrom
darookee:remove-set-_GET-in-sArticle

Conversation

@darookee
Copy link
Contributor

In sArticles.php wird die _GET Variable in der Funktion sGetArticleById überschrieben, was zu unvorhersehbaren ergebnissen führen kann, dies sollte es beheben.

In sArticles.php wird die _GET Variable in der Funktion sGetArticleById überschrieben, was zu unvorhersehbaren ergebnissen führen kann, dies sollte es beheben.
@darookee
Copy link
Contributor Author

Wieso werden hier eigentlich nicht die Models verwendet?

@sthamann
Copy link
Contributor

Hi,

"Wieso werden hier eigentlich nicht die Models verwendet?"

Der Code in den Klassen unter engine/core/class/ ist "Deprecated" und wird mit den kommenden Releases Step by Step durch Models abgelöst.

Ein Problem gibt es in deinem Pull-Request - in der Methode wird ja derzeit die Category-Id teilweise direkt aus dem Request geholt, man müsste also zusätzlich einen Parameter categoryId an die Methode anfügen und im Controller die Übergabe ergänzen.

Kannst du das ggf. noch ändern? Dann kann ich den Pull-Request mergen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hier wird nur noch $sCategoryID für die lokale Verwendung gesetzt, $_GET['sCategory'] wird also nicht mehr überschrieben.

@darookee
Copy link
Contributor Author

Moin!

Erstmal danke für die Info. Ich dachte für die 4.0 war das schon abgeschlossen. Wie sieht das denn dann mit der Modelerweiterung aus? Die Daten, die dort erweitert werden, sind dann wohl nicht ohne weiteres auf der Artikeldetailseite verfügbar...?

Dann zum Pullrequest:

Kurz über der oben Kommentierten Zeile wird ja die $sCategoryID gesetzt, anhand des übergebenen Parameters. Im weiteren verlauf der Funktion wird (wenn ich nichts übersehen habe) nur noch diese Variable verwendet, $_GET wird also nicht mehr überschrieben.
Das Problem was ich hier ändern wollte war folgendes: Wenn man z.B. ein Plugin hat, was mit Hilfe dieser Funktion Artikeldaten ausliest und dieser Artikel nicht in der zur Zeit angewählten Kategorie ist, dann kann es sein, dass z.B. die Menugenerierung (Untermenus) nicht mehr richtig funktioniert. Im Konkreten Fall war es so, dass auf der Startseite immer das letzte Menu ausgeklappt war, weil ein Artikel aus dieser Kategorie angezeigt/ausgelesen worden ist.
Als Workaround hatten wir es dann bisher so gehandhabt, dass vor dem aufrufen ein 'Backup' von _POST, _GET und _SESSION gemacht worden ist (siehe Hier). Dies sollte aber mit diesem Commit gelöst sein.

Wenn es wirklich so sein soll kann ich die Funktion aber gerne noch erweitern und als Fallback für den Parameter es so handhaben wie es zur Zeit auch ist.

@klarstil
Copy link
Contributor

Hallo darookee,

ich habe deinen Pull-Request gemergt. Danke, dass du als Contributor an Shopware aktiv mitwirkst. Wir haben unseren Workflow noch ein wenig angepasst.

Für alle Contributor Pull-Requests haben wir jetzt einen Community-Branch, wohin alle Pull-Requests gesammelt werden. Bitte sende deshalb deinen nächsten Pull-Request an den Community-Branch und nicht an den Master-Branch.

@klarstil klarstil closed this Sep 20, 2012
@lennartdiedrich
Copy link

Hallo klarstil,

könntest du den Workflow noch etwas erläutern? Wird der Community Branch dann in den master gemerged? Wenn ja, wir regelmäßig? Arbeitet ihr intern weiter mit svn und synchronisiert das svn Repository dann "ab und zu" in den master? Vielen Dank für die Info.

@klarstil
Copy link
Contributor

Hallo lennartdiedrich,

klar, kein Problem. Also der Workflow sieht bei uns wie folgt aus. Wir haben zwei Branches master und Community-Branch.
Der Community-Branch-Branch ist für alle Contributor gedacht, die an Shopware aktiv mitarbeiten wollen. Um die Änderungen sauber übernehmen können, wäre es gut wenn alle Pull-Requests an den Community-Branch-Branch gesendet werden.
Bevor wir die nächste Minor-Version von Shopware 4 erstellen, werden wir dann alle Pull-Requests im Community-Branch validieren, mergen und noch mal durch unsere QA jagen.

Ein Beispiel: Du contributest für Shopware 4.0.2, dann werden wir deine Änderungen, vorausgesetzt das Review war erfolgreich und die QA hat nichts zu bemängeln, in Shopware 4.0.3 ausliefern.

Intern arbeiten wir noch mit SVN und synchronisieren die Änderungen jeden Tag mit den master-Branch. Ich habe aber auch alle Informationen in die README.md eingetragen, welche beim nächsten Sync aktualisiert wird.

@klarstil klarstil reopened this Sep 20, 2012
sthamann pushed a commit that referenced this pull request Sep 20, 2012
sthamann pushed a commit that referenced this pull request Sep 20, 2012
@OliverSkroblin
Copy link
Contributor

Hallo lennartdiedrich,

wir würden gerne deinen Pull Request in unseren Master Branch übernehmen. Könntest du bitte die gitignore datei aus dem Pull Request entfernen.

Mit freundlichen Grüßen

Oliver Denter

@lennartdiedrich
Copy link

Hallo Oliver,

der PullRequest ist von @darookee, daher kann ich ihn nicht bearbeiten. :)
@darookee wird euch aber sicher helfen... ;)

Schöne Grüße,
Lennart

@darookee
Copy link
Contributor Author

darookee commented Oct 4, 2012

Bidde... ^^

@OliverSkroblin
Copy link
Contributor

Vielen dank fürs entfernen der gitignore Datei.

Wir haben die Pull Request gemergt. Sobald dieser noch einmal durchgetestet wurde, wird dieser global zur Verfügung gestellt.

Vielen Danke nochmals

Schöne Grüße
Oliver Denter

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.

5 participants