-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Feature: CodeIgniter 4 upgrade #3592
Conversation
8fa7b51
to
d81ce55
Compare
I've created a draft PR just to compare the two branches. We are going to need to write a special upgrade instructions document for this upgrade. @odiea do you care to do this? It will need to include things like manually moving images in item_pics, etc to the new location... |
Does this new CI4 branch include the changes from the master that were merged since the original codeigniter_4_upgrade branch was created? Those are going to manually need to be ported to the CI4 format. If they are blindly merged in then they won't work because the syntax for many things is different. |
@jekkos did we decide whether this version will be 3.4.0 or 4.0.0? |
@objecttothis I always thought to stick with 3.4.0, as we do not really add new features. But this might not be reflecting the fact that the upgrade was a huge effort. |
That's fine. I'll push a commit. |
- correct URL - Removed line endings - Removed unneeded include
- Removed esc() call that was causing problems - Renamed function to match CI4 expectations - String interpolation - corrected URL for pic_thumb
322e357
to
246ea27
Compare
@objecttothis |
Thank you. I will replace those instances tomorrow when I am back in the office. |
Thank you @objecttothis for your quick response ho to perfect the fix link the.zip file attached to my message from ********** START debug ************* // IN /root/ci4_opensourceospos_latest/public/config `// IN /root/app/Controllers/Config.php
`// INFOS: https://stackoverflow.com/questions/58728648/how-to-display-uploaded-images-in-codeigniter-4 // IN /root/app/Controllers/Config.php -> error 404, rename remove_logo TO postRemove_logo
` |
@DEV-byoos thank you for the helpful tips. Please check out ci4-branch, make the changes there and submit a pull request against ci4-branch for these changes. I can do a code review at that time, but wading through the shorthand in your comment is difficult. |
@objecttothis should we reopen the pull request now with the new branch instead? |
@DEV-byoos indeed it might be better if you submit your fixes in a pull request. Do you know how to do that? Thanks! |
I think you can edit this PR so it's ci4-branch trying to merge into master instead of ci4-upgrade. It would be a shame to lose the conversation history in this PR by creating a new one. |
@objecttothis hello, Well, thank you for your suggestion but I'm not a github boss and my Internet network is weak.
|
@owlbrudder @jekkos do either of you want to take the time to look through @DEV-byoos changes and implement them? @DEV-byoos I'm working through fixing some problems with attributes right now. I'm sorry, but I just don't have time to submit these changes myself. The formatting does not come through very well in github because you aren't using ``` before and after your code blocks. Your changes are probably very helpful, but I can't get to it. I don't know what your first language is, but I recommend reading a tutorial on how to create a branch, make changes and submit them as a pull request. You can even do these by hand in Github if you need to. |
This looks like something I could use to get back into working state. I am
near end of day here, but I could have a look at it in the morning if
that's okay?
…On Mon, 6 Nov 2023, 5:25 pm objecttothis, ***@***.***> wrote:
@objecttothis <https://github.com/objecttothis> hello,
Well, thank you for your suggestion but I'm not a github boss and my
Internet network is weak. While I get used to the pull-request I suggest
sending you patch by patch on "Feature: CodeIgniter 4 upgrade #3592
<#3592>" you can test
then update the ci4-upgrade, OK? like /root/app/controllers/Config.php ---
remove_logo method FIX rename remove_logo with postRemove_logo code `
public function postRemove_logo(): void { $file = FCPATH.'uploads/' .
$this->config['company_logo']; if (is_readable($file) && unlink($file)) {
$success = $this->appconfig->save(['company_logo' => '']); } else {
$success = "The file was not found"; } $success =
$this->appconfig->save(['company_logo' => '']);
echo json_encode (['success' => $success]);
}`
@owlbrudder <https://github.com/owlbrudder> @jekkos
<https://github.com/jekkos> do either of you want to take the time to
look through @DEV-byoos <https://github.com/DEV-byoos> changes and
implement them? @DEV-byoos <https://github.com/DEV-byoos> I'm working
through fixing some problems with attributes right now. I'm sorry, but I
just don't have time to submit these changes myself. The formatting does
not come through very well in github because you aren't using \``` before
and after your code blocks. Your changes are probably very helpful, but I
can't get to it. I don't know what your first language is, but I recommend
reading a tutorial on how to create a branch, make changes and submit them
as a pull request. You can even do these by hand in Github if you need to.
—
Reply to this email directly, view it on GitHub
<#3592 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AE2THY7F5ODY2MCVURT75JLYDCUIBAVCNFSM6AAAAAAR7HQVR2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOJUGM4TMNRZGM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
DAY 1 following explanation:
PS I use code tags, I don't understand why it's buggy??? OK I found thanks @objecttothis |
Thanks for the explanation. I will look at it all in the morning and get
back to you.
…On Mon, 6 Nov 2023, 6:58 pm BYOOS, ***@***.***> wrote:
thanks @owlbrudder <https://github.com/owlbrudder>
following explanation:
the company_logo config option allows you to replace the OSPOS logo with
the company logo, OK
1.
Add company logo YES
2.
validate the config modification YES after fix
3.
delete the logo NO because it does not disappear on disk, only in the
database TABLE ospos_app_config KEY company_logo
4.
if we reload a new company logo the name on the disk will be indexed
filename_1...
NOTE company logo can only be loaded once.
ike /root/app/controllers/Config.php --- remove_logo method
FIX rename remove_logo with postRemove_logo
code
` public function postRemove_logo(): void
{
$file = FCPATH.'uploads/' . $this->config['company_logo'];
if (is_readable($file) && unlink($file)) {
$success = $this->appconfig->save(['company_logo' => '']);
} else {
$success = "The file was not found";
}
$success = $this->appconfig->save(['company_logo' => '']);
echo json_encode (['success' => $success]);
}`
PS I use code tags, I don't understand why it's buggy???
best regards
—
Reply to this email directly, view it on GitHub
<#3592 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AE2THYZTRA4FSMUUJURYCFTYDC7G3AVCNFSM6AAAAAAR7HQVR2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOJUGU3DEMJYGI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
@DEV-byoos indeed it might be better if you submit your fixes in a pull request. Do you know how to do that? Thanks! Answer @DEV-byoos |
Ca sera bon si tu apprendrais a faire un requet tirer. Ou autrement vous pouvez justement envoyer un diff generé par git. Je pense que ca va etre plus facile a inspecter; |
I checked this, you can only change the target branch, but not the source. Which might make sense. The conflicts we see here are resolved in the other branch. I can copy the content of the issue list here to a new PR. |
Thanks for the proposition. Regarding the function rename, I would not mix camelcase with snake case. In this case camelcase is the way to go. |
DAY 2 following explanation:
coded
the $file_info = [...] block is moved up and an 'error' key is added to initialize the value to empty. |
@DEV-byoos this comment section is going to get long and chaotic. Please use Element (formerly glitter). You can use their online client or download a client on their website (https://matrix.org/ecosystem/clients/element/) just change the server to We will be closing and deleting this PR soon since the other PR is what will be merged with the master. post a message to opensourcepos/Lobby and I'll send the invite to opensourcepos/devroom |
I'm closing this PR as it will not be one we will merge. See #3858 |
Upgrading CodeIgniter to 4.4.3 #842
Here are a list of issues that need to be resolved: