Skip to content

Conversation

geelweb
Copy link

@geelweb geelweb commented Sep 8, 2025

Hi,

I'm trying to get rid of our 10 years old fork migrating legacy code under php8.4 to switch on new servers... Everything works fine if :

  • I skip the checkPlaceholderExists test...
  • I set preserve_input option to false

In this PR, I added a check_placeholder_exists option to skip the check, and a static setGlobalOptions method to define options globaly for all future instances.

Hope you can accept this PR, it will be a real cost saving for me .

@duboism
Copy link
Contributor

duboism commented Sep 9, 2025

I know the pain (that's exactly how I get involved with this project). However, could you explain more what problem(s) you are trying to solve ? Do you think it's a bug in the library or does your application needs an improvement ?

@geelweb
Copy link
Author

geelweb commented Sep 9, 2025

I don't think the library is bugged, I think If I fix our templates and the usage of the lib (ensure setCurrentBlock, setVariable, parsCurrentBlock, parse... are called correctly according to the template content), it will works well with the current version.

But adding an option to skip the checkPlaceholderExists test will fix most of my issues, saving me a lot of time.

And with the second modification I made to globally define the default options, I can also skip the update of the hundreds of HTML_Template_IT instantiation.

With this 2 new features, BCBreak free, I can upgrade to the official package simply adding in my code

HTML_Template_IT::setGlobalOptions([
    'preserve_input' => false,
    'check_placeholder_exists' => false  
]);

So I don't fix any bugs in HTML_Template_IT, it's more a kind of improvement to make migration easier for developers who start using the lib more than 20 years ago ;)

@duboism
Copy link
Contributor

duboism commented Sep 26, 2025

Sorry for the long delay. I'm trying to understand if your modifications made sense here or if they too strongly to your usage (in which case, you can create a subclass - fixing the call to the constructor should be easier than fixing the calls to the template).

Can you explain why do you need to skip checkPlaceholderExists ?

Also, I'm not sure you need to add the setGlobalOptions class method (since the default value of $_options['check_placeholder_exists'] is true). In fact, IIUC your code, you don't use $globalOption to bypass the test. So maybe this can be split in 2 PR (one to add an option to bypass checkPlaceholderExists, one to introduce setGlobalOptions).

@geelweb
Copy link
Author

geelweb commented Sep 29, 2025

Can you explain why do you need to skip checkPlaceholderExists ?

I need to skip this check because I have hundreds (395 exactly) of templates created before this check was introduced

Also, I'm not sure you need to add the setGlobalOptions class method (since the default value of $_options['check_placeholder_exists'] is true)

The default value of the new option is true to not BCBreak the lib. In my case instead of update hundreds of new HTML_Template_IT call to add the options, I use setGlobalOptions in my "bootstrap"

HTML_Template_IT::setGlobalOptions([
    'preserve_input' => false,
    'check_placeholder_exists' => false
]);

I understand this is not really usefull for the lib, I can keep the fork if you reject.

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.

2 participants