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

Update uninstall.php #4

Merged
merged 1 commit into from
Apr 23, 2018
Merged

Update uninstall.php #4

merged 1 commit into from
Apr 23, 2018

Conversation

schuer
Copy link
Member

@schuer schuer commented Apr 23, 2018

No description provided.

@schuer schuer requested a review from polarpixel April 23, 2018 11:37
@polarpixel polarpixel merged commit b5fc63c into master Apr 23, 2018
Copy link
Member

@polarpixel polarpixel left a comment

Choose a reason for hiding this comment

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

Danke!

@schuer schuer deleted the schuer-patch-1 branch April 23, 2018 12:45

// Computes the difference of two arrays recursively
// https://gist.github.com/t3chnik/6b3b14d3859d810c02f4
function array_diff_recursive($aArray1, $aArray2)
Copy link
Member

Choose a reason for hiding this comment

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

Die Funktion mit diesem sehr allgemeinen Namen einfach so im globalen Namespace zu deklarieren, finde ich nicht so gut.
Kann durchaus passieren, dass die Funktion schon woanders definiert ist. Zum Beispiel wenn die in mehreren Addons in der uninstall ist, und es mal eine Situation gäbe, wo mehrere Addons gleichzeitig deinstalliert werden (Ok, gibt es aktuell glaube ich nicht), dann würde es krachen.

Man könnte ein function_exists drum machen. Dann muss man sich aber auch drauf verlassen, dass falls sie schon definiert ist, sie genauso arbeitet.
Alternativ könnte man einen spezifischeren Funktionsnamen nehmen. Oder mit einer anonymen Funktion, wobei das bei rekursiven Funktionen etwas tricky ist:

$arrayDiffRecursive = function (aArray1, aArray2) use (&$arrayDiffRecursive) {
    // ...
    $arrayDiffRecursive(...);
    // ...
}

Copy link
Member

Choose a reason for hiding this comment

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

oder einen namespace verwenden.

Copy link
Member Author

Choose a reason for hiding this comment

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

Zum Hintergrund: Die install.php und uninstall.php werden in allen Demos auf gleiche Art verwendet und sollten einfach hin- und herkopierbar sein, ohne AddOn-spezifische Anpassungen vornehmen zu müssen (wie Namespaces). Wenn ihr eine gute Lösung habt, bringt sie gerne ein. Ich bin nicht so firm im Thema, um das sicher zu lösen.

Copy link
Member

Choose a reason for hiding this comment

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

wenn du sie 1:1 kopieren willst, würde ich die funktion in eine lokale variable schreiben - wie gregor oben vorschlägt.

@tbaddade
Copy link
Member

auf gleiche Art verwendet und sollten einfach hin- und herkopierbar sein

Weshalb ist das notwendig?

@schuer
Copy link
Member Author

schuer commented Apr 24, 2018

Weshalb ist das notwendig?

Die Demo-AddOn-Entwickelnden wurde von mir etwas mit der Installationsrouting erschlagen. Sie kam irgendwann hinzu und macht einige Verrenkungen, die man nicht auf Anhieb versteht (vor allem die Sache mit dem Split der package.yml). Ich wollte vermeiden, dass man pro AddOn nun auch noch Anpassungen in der Routine vornehmen muss, sondern es sollte stattdessen zumindest generisch funktionieren, wenn man den Kram schon im AddOn haben muss und nicht als separates AddOn/PlugIn dazu bekommt.

Langfristig wäre aber nachwievor sinnvoll, dass der REDAXO-Installer die Funktionalität nativ anbietet, so dass die Funktionen nicht mehr in den AddOns abgebildet werden müssen.

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