php 8.1 compatibility #711
Conversation
|
If it contains all changes, maybe you can keep the commit history, at least for the contributor who did the original PR 😉 |
If you can tell me, how to do that, I would be happy to keep the author. The only thing I know is cherry picking. But as there are some changes in at least two commits we don't want to keep, I'm not sure to achieve it. I could cherry pick two of the four changes without any required modifications. Seemed to be much less effort to me to simply copy the changes. But if there is an easy way, I'd be happy for an advice. |
You can retrieve the branch with |
| @@ -196,8 +196,8 @@ public function clear(Smarty $smarty, $resource_name, $cache_id, $compile_id, $e | |||
| */ | |||
| public function hasLock(Smarty $smarty, Smarty_Template_Cached $cached) | |||
| { | |||
| clearstatcache(true, $cached->lock_id); | |||
| if (is_file($cached->lock_id)) { | |||
| clearstatcache(true, $cached->lock_id ?? ''); | |||
There was a problem hiding this comment.
you probably don't want to call clearstatcache with empty filename. it's performance killer to clear all cache and looking at the immediate code around this call, doesn't seem you need "clear all cache"
perhaps re-evaluate clearstatcache usage at all, as it's not cached for inexistent files and unlink invalidates automatically:
There was a problem hiding this comment.
also, I believe this change should be extracted to own pull request, not related to 8.1 per se, but affects all (currently supported) versions
There was a problem hiding this comment.
I'm replying here, but I've closed the pr because of cherry-picking the commits from the previous author. Personally I agree with your point, but IMO it's out of scope of this change. What the original author tried to do is "doing whatever php < 8.1 would do". So another pr should cope with this issue.
There was a problem hiding this comment.
you can still split the commits, fix commit message.
and to preserve authorship (keep credits), add "Co-Authored-By: xxx" git trailer (github renders them ok)
There was a problem hiding this comment.
Thanks, good to know. I've done something wrong, I guess and now I'm having some commits twice and a merge commit. Looks like I did it wrong using merge instead of using rebase?
Shall we continue here or on the other pr regarding your original comment to clearstatcache?
I'll try to come up with another branch/pr with at least the two commits as being cherry picked. I'm not sure how to handle the first commit, as there is already a change regarding the IntlDate-Stuff which we want to skip here. For now I've set the pr as draft. |
|
I'm closing this pr as I've created #713 as a follow-up. |
can you explain it? show what error you got? |
It's just missing knowledge, I guess. I have no clue, how to remove commits that are already pushed for example. And in this branch I've just copied all changes into my editor and create new commits. |
|
seems also a lack of knowledge to share error messages? :D since don't know what step you got stuck, here's the full flow:
you can use |
Looks like :-D Just joking, as I did not perform any commands, there was no chance to send any error message. So what would be the correct process to change what I did? I started from a fresh clone of the smarty-repo and created a new branch. Using my local ide, I've copied all changes from the github-ui and created a new commit with the changes. After the feedback in this pr to keep the author, what should I have done to adjust the commits? |
|
@glensc Ah, I think I've got it now. The push force allows me to change whatever the branch contains on the server. That's not allowed in our on-premise environment, therefore I've never used it. ;-) |
|
force push is typically allowed in pull request, but not on main branches |
This pr contains all changes from #678 except for the IntlDateTime related stuff. I only kept the change from strftime to date in internal functions.