rapila / cms-base

The rapila cms’ internals. Please file all issues here if they’re not directly related to a plugin or the sample site.
http://www.rapi.la
3 stars 1 forks source link

Allow multiple use of page identifier #263

Closed juergmessmer closed 3 years ago

juergmessmer commented 3 years ago

• for example for replacing meta robots and googlebot, maybe there could be other usages

Like this different identifiers could be replaced by different page_properties or the same. More flexibility, and easier to administer.

In case nobody sees negative consequences, I would like to merge this into master.

sabberworm commented 3 years ago

What are shared page identifiers? I think you mean page properties, not page identifiers. There should only be a single identifier, no?

juergmessmer commented 3 years ago

Simply use the same PageProperty=name identifier twice, as for example to replace the meta “robots” and “googlebot” with the same setting, instead of having to administer two properties in admin

Now the db entry is reset if it’s used twice. That’s why I suggest the fix.

On 1 Feb 2021, at 09:37, Raphael Schweikert notifications@github.com wrote:

What are shared page identifiers? I think you mean page properties, not page identifiers. There should only be a single identifier, no?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/rapila/cms-base/pull/263#issuecomment-770946141, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAC3NIN353B27TUTR54NOH3S43DCJANCNFSM4W4XJO4Q.

juergmessmer commented 3 years ago

What are shared page identifiers? I think you mean page properties, not page identifiers. There should only be a single identifier, no?

I changed this text, hopefully clearer now!

juergmessmer commented 3 years ago

If this suggestion doesn't make sense for a reason that I am not aware of, I suggest we'd add an exception, in case the same PageProperty name is used in a template, because in this case it simply doesn't work, since the db-value is reset to null. Took me a while to figure that out.

sabberworm commented 3 years ago

Sorry, aber ich versteh einfach voll den Unterschied nicht. $aAvailablePageProperties sind ja einfach alle Properties, die im Template verwendet werden. Ohne das isset werden davon einfach immer die letzten ins $aResult geschrieben, sonst die ersten.

juergmessmer commented 3 years ago

Wenn zum der PageProperty=robots zwei mal verwendet wird, so wird beim ersten Mal in der Schleife von $aAvailablePageProperties dann anschliessend der Datenbankeintrag - unset($aSetProperties[$sPropertyName]) - entfernt, und beim nächsten Mal dann mit null ersetzt. Also im PageDetail PageProperty wird dann der DB Wert nicht angezeigt, und beim nächsten speichern gelöscht.

On 1 Feb 2021, at 11:42, Raphael Schweikert notifications@github.com wrote:

Sorry, aber ich versteh einfach voll den Unterschied nicht. $aAvailablePageProperties sind ja einfach alle Properties, die im Template verwendet werden. Ohne das isset werden davon einfach immer die letzten genommen, sonst die ersten.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/rapila/cms-base/pull/263#issuecomment-771031704, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAC3NIJF6DPNOTZNX4ZJJVLS43RZRANCNFSM4W4XJO4Q.

sabberworm commented 3 years ago

Ah, jetzt versteh ich. Das Problem liegt nicht eigentlich am Block der neu im if eingeschlossen liegt sondern am unset untendran.

Ich schlag vor wir krempeln das ganze etwas um: zuerst schreiben wir die Properties aus dem Template ins $aResult und dann ergänzen wir die Daten aus der DB. Dann brauchen wir keine dritte Schleife ($sRemainingPropertyName) und der Code wird lesbarer da die Intentionen klarer sind.

juergmessmer commented 3 years ago

Done 39a279bb17e78c32e0f2c15a407f6730e0be97ea

Merge to master?

juergmessmer commented 3 years ago

Ok, danke!

On 6 Feb 2021, at 16:21, Raphael Schweikert notifications@github.com wrote:

@sabberworm commented on this pull request.

In modules/widget/page_detail/PageDetailWidgetModule.php https://github.com/rapila/cms-base/pull/263#discussion_r571497008:

    }
  • foreach($aSetProperties as $sRemainingPropertyName => $sRemainingPropertyValue) {
  • $aResult[$sRemainingPropertyName] = array('value' => $sRemainingPropertyValue, 'defaultValue' => null, 'type' => null);
  • foreach($oPage->getPagePropertyQuery()->byNamespace(false)->find() as $oPageProperty) {
  • if(isset($aResult[$oPageProperty->getName()])) {
  • $aResult[$oPageProperty->getName()]['value'] = $oPageProperty->getValue();
  • } Ausserdem hat der alte Code so funktioniert.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/rapila/cms-base/pull/263#discussion_r571497008, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAC3NIMXBHWSTDKS3KZS6DLS5W6IPANCNFSM4W4XJO4Q.