Simplified persistent templates: removed RWD's ones and merge differences in base

[justinbeaty]

For some reason, the RWD theme sets some templates to the ones in template/persistent without checking ifconfig.

I have not tested this yet. None of the templates that are now used (i.e. customer/form/register.phtml) exist in the RWD folder, so they fallback to base. There are some visual differences between the templates so it's possible this breaks something in RWD.

[justinbeaty]

An example of something that might break. This HTML isn't in app/design/frontend/base/default/template/customer/form/register.phtml

maho/app/design/frontend/rwd/default/template/persistent/customer/form/register.phtml

Lines 172 to 174 in 2285ea2

	<?php if (Mage::helper('checkout')->isContextCheckout()): ?>
	<input name="context" type="hidden" value="checkout" />
	<?php endif ?>

[fballiano]

what if we migrate the difference between rwd/default/layout/persistent.xml and base/default/layout/persistent.xml into base/default/layout/persistent.xml and remove rwd/default/layout/persistent.xml?

[justinbeaty]

what if we migrate the difference between rwd/default/layout/persistent.xml and base/default/layout/persistent.xml into base/default/layout/persistent.xml and remove rwd/default/layout/persistent.xml?

Something like this, yeah. I also wonder if we can get rid of templates such as persistent/customer/form/register.phtml and migrate any changes into the regular customer/form/register.phtml file. Would just need to check if persistent is enabled around some elements.

[fballiano]

exactly that would be lovely, it happened to me many time to start debugging a template only to find out that I had to work on the persistent one or the other way around.

do you want to keep working on this?

[justinbeaty]

Yeah, I'll work on this because it will simplify the EAV PR.

[fballiano]

beautiful!

@justinbeaty can I add return types to public methods instead of the docblock?

[justinbeaty]

beautiful!

@justinbeaty can I add return types to public methods instead of the docblock?

Yes, go ahead. I must get in the habit of doing that myself.

Will do RWD theme later today...

[fballiano]

I'm wondering, can't we just remove the persistent/* templates and merge the features into the other ones with some "if persistent enabled"?

[justinbeaty]

@fballiano One thing about removing the template files in template/persistent, if a user has modified these files in their theme, we would no longer load them and instead be loading the templates in template/customer/form, etc.

I added a simple method to Mage_Core_Block_Template that sets the template only if it exists. What do you think about it?

[justinbeaty]

I'm wondering, can't we just remove the persistent/* templates and merge the features into the other ones with some "if persistent enabled"?

Yes, I already have done that for the base theme, but see previous comment.

[fballiano]

@fballiano One thing about removing the template files in template/persistent, if a user has modified these files in their theme, we would no longer load them and instead be loading the templates in template/customer/form, etc.

mmmm (I'm thinking) we could add a check to the health-check script (if there's a app/design/*/persistent then alert) and put it in the release notes, because the "settemplateifexists" has to check on the filesystem to find the files across the fallback system only if somebody didn't update their theme. at this stage of the project I think we can do this.

but let's think about it

[justinbeaty]

The health-check script is a good idea, and the solution is simple for the end user: they just need to move the files from template/persistent/ to template/customer/form/ and template/checkout/onepage/ in their theme.

But I did a quick benchmark, when loading example.com/customer/account/login we run an if(file_exists()) check 754 times in the file functions.php. Using the setTemplateIfExists method would add 2-3 more checks only if the persistent template doesn't exist.

So it's not a huge performance loss, but if we can avoid it then it's better. It's also possible to use the fallback for the first release of Maho, and drop it in future ones, but as you said that's probably not needed at this stage of the project.

I'm not sure if the setTemplateIfExists method is useful anywhere else... Maybe as a PR that implements functionality like found in Aoe_Layout.

Anyway, I'll leave the commit for now, but feel free to make the final decision to revert it.

[justinbeaty]

@fballiano Why remove the rwd persistent templates? There are differences between them and the base theme.

[justinbeaty]

Also I don't think the remember me popup is broken, but it's definitely an outdated concept so I'm not against removing it.

[fballiano]

aren't the differences only estetical? I tested this version without the templates and it seems to me to look the same

[fballiano]

aren't the differences only estetical? I tested this version without the templates and it seems to me to look the same

I was clicking on it and nothing happens and there's no "dom structure" for it to show nor anything in the developer console.

anyway everybody knows what a "remember me" is today.

[justinbeaty]

aren't the differences only estetical? I tested this version without the templates and it seems to me to look the same

The RWD ones are more complete, they do things like checking is users are allowed to register, have better form field autocompletes, and some other things. I am in the process of migrating some of those changes to the base theme anyway.

We can get rid of the RWD ones if you'd like, but there will be some RWD theme specific stuff going in there (such as classnames on html elements.)

[fballiano]

The RWD ones are more complete, they do things like checking is users are allowed to register, have better form field autocompletes, and some other things. I am in the process of migrating some of those changes to the base theme anyway.

ok i'll revert it

We can get rid of the RWD ones if you'd like, but there will be some RWD theme specific stuff going in there (such as classnames on html elements.)

from what I've seen the end result seemed identical, and if it's just class names then I'd fix the layout in the CSS instead of having a duplicated template

[justinbeaty]

from what I've seen the end result seemed identical, and if it's just class names then I'd fix the layout in the CSS instead of having a duplicated template

I'll combine it all into the base theme files, but will finish tomorrow.

[fballiano]

I’ll also try to sleep now 😊😊 Il giorno lun 30 set 2024 alle 00:35 Justin Beaty ***@***.***> ha scritto:

…

from what I've seen the end result seemed identical, and if it's just class names then I'd fix the layout in the CSS instead of having a duplicated template I'll combine it all into the base theme files, but will finish tomorrow. — Reply to this email directly, view it on GitHub <#34 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAG6DLZDSIPKD7MVQDP265DZZCFFTAVCNFSM6AAAAABPAXBN3SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGOBRGY2TENJWHE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[justinbeaty]

Merged all the functionality into the base templates. Need to test combinations of isRegistrationAllowed, isAllowedGuestCheckout, and isPersistentEnabled.

[justinbeaty]

I've tested everything I can think of, and all is working. All that is left is to decide about the setTemplateIfExists function.

[justinbeaty]

As a side note, I think the whole Mage_Persistent module is pretty outdated as a concept, and could be removed entirely. Why have two cookies, where one (om_frontend) expires sooner, but the other (persistent_shopping_cart) remains keeping you in a semi-logged in state? Can't we just make the "Keep me logged in" box extend the lifetime of the regular cookie? Before I went headless with my store, I recall it always causing my customers problems.

Nothing in this PR would be wasted if we went that direction, instead it was a necessary first step.

[fballiano]

Thank you @justinbeaty, such an amazing job like everything you did.

Completely agree that the whole persistent module is a thing of the past and the "extend cookie lifetime" is actually a simple and very cool idea!

I was wondering about the setTemplateIfExists, on one hand I don't love ambiguity (it's either this template or the other) but in this case it would be a way to be backward compatibile with somebody who customized those templates so.. if you like that functionality I'm ok with leaving it and merging it as it is :-)

[justinbeaty]

Thank you @justinbeaty, such an amazing job like everything you did.

Completely agree that the whole persistent module is a thing of the past and the "extend cookie lifetime" is actually a simple and very cool idea!

I was wondering about the setTemplateIfExists, on one hand I don't love ambiguity (it's either this template or the other) but in this case it would be a way to be backward compatibile with somebody who customized those templates so.. if you like that functionality I'm ok with leaving it and merging it as it is :-)

Actually, I think let's get rid of the setTemplateIfExists from this PR and keep it in mind for the future if it will make any backwards compatibility easier for anything else.

The reason I say that now is because if we remove the persistent module, then we don't want to load templates that call a non-existent function, i.e. here: https://github.com/MahoCommerce/maho/blob/main/app/design/frontend/rwd/default/template/persistent/customer/form/login.phtml#L46

Let me remove that call, and then we can merge. Then I can make a PR for removing the persistent module, as I think it will be pretty simple. Then back to the EAV module. :)

[fballiano]

look at this commit 2285ea2 becoming a PR in openmage OpenMage/magento-lts#4244 😂

[justinbeaty]

look at this commit 2285ea2 becoming a PR in openmage OpenMage/magento-lts#4244 😂

Hm, yes surely this PR prompted that, but the very first comment in this PR mentions that it will break things without template changes. The magento devs severely neglected the base templates.