search

RedDragonWebDesign / BlueThrust5

A fork/update of the famous BlueThrust Clan Scripts v4. Gaming community website engine in PHP.
10 stars 7 forks source link

Cleans up a bunch of warnings, errors and form issues when adding dow… #115

Closed deepend-tildeclub closed 8 months ago

deepend-tildeclub commented 8 months ago

Added some missing files. Many fixes relating to Warnings, Depreciation messages. Fixed adding a download the drop down for the download section only listed the 1st section that was above the Forum Attachment category.

RedDragonWebDesign commented 8 months ago

So this repo now has something called "continuous integration" or "CI". These are automated tests that are run on every patch and every commit. You can see if they pass or not by looking for the little green check mark (pass) or the little red X (fail) in various places.

For PRs, this is located in the section that says "Some checks were not successful".

image

You'll want to click on "details" by the one that didn't pass for a list of the problems it's detecting.

The one failing here is PHP Code Sniffer, which is our linter. A linter is a program that enforces consistent code style. This will keep problems that I've fixed in the other files from creeping back into the repo in new patches.

After you look up the details and see something like this...

image

...there are two ways to fix it. 1) Is to pay attention to the file name, line number, and issue, and go do it manually. 2) Is to run composer install, then composer exec phpcbf -p ., which will auto fix everything. Either method you use, save and commit, and the CI checks will re-run.

In general, CI checks need to all be passing to merge a patch. Hopefully that all makes sense!

deepend-tildeclub commented 8 months ago

...there are two ways to fix it. 1) Is to pay attention to the file name, line number, and issue, and go do it manually. 2) Is to run composer install, then composer exec phpcbf -p ., which will auto fix everything. Either method you use, save and commit, and the CI checks will re-run.

composer says -p option doesn't exist.

RedDragonWebDesign commented 8 months ago

My bad. Try it with quotes around it. composer exec "phpcbf -p ."

deepend-tildeclub commented 8 months ago

There is an issue I have fought with. In members/include/admin/managedownloadcat/main.php line 37 I could not get that path to work properly unless I put a full path from / . Couldn't figure out why any relative path I put in that wouldn't work.

RedDragonWebDesign commented 8 months ago

This PR is probably too big. Will probably need to be split. Unless all these 45 changed files are only affecting one page or one feature.

It's industry standard to manually test PRs before approving them. If PRs are too big, it becomes hard to test everything.

Added some missing files.

Where did you get the missing files from? How did you know they were missing?

Fixed adding a download the drop down for the download section only listed the 1st section that was above the Forum Attachment category.

What page should I visit and what am I looking for when testing? Thanks.

deepend-tildeclub commented 8 months ago

This PR is probably too big. Will probably need to be split. Unless all these 45 changed files are only affecting one page or one feature. It only jumped up after I ran that composer command. The actual files I changed you can see on the commit.
https://github.com/deepend-tildeclub/BlueThrust5/commit/5cd75c5b283b538c50f80f6e2194249e99a2f81b

Where did you get the missing files from? How did you know they were missing?

I found them in the older release of the software. I knew they were missing because the files were referenced in some errors I was getting when dealing with the download section issues.

What page should I visit and what am I looking for when testing? Thanks. For this specific issue it was when you were in /src/members/console.php?cID=145 it wouldn't show all the "Sections" that were added. But I checked against master now to get the information you seek and the issue is not there. The change for adddownload.php relating to "$downloadcatoptions" is what I change to get the menu working when I made the change. Not sure if something else got pushed to master between when I was working on that and when I tested now.

Thanks :)

RedDragonWebDesign commented 8 months ago

This patch is too big and it'd be a lot of work to make you split it, so I am going to do some splitting myself.

I've split everything on the "Manage Download Categories" page into a6cf7f2. May do more splitting as I immerse myself in this patch more.

In the future, please write smaller patches. Big patches are hard to review. Please also include clear test instructions.

deepend-tildeclub commented 8 months ago

This patch is too big and it'd be a lot of work to make you split it, so I am going to do some splitting myself.

I've split everything on the "Manage Download Categories" page into a6cf7f2. May do more splitting as I immerse myself in this patch more.

In the future, please write smaller patches. Big patches are hard to review. Please also include clear test instructions.

Only figured it wouldn't be too bad as the changes were mostly small and they were related. I'll keep it down to single files to avoid issues. Also some of my issue on these patches were that I don't think my git was updating properly. So you weren't getting the same results.

If you want to trash this PR from this point I'm OK with it. I can go and do the fixes again and submit them individually.

RedDragonWebDesign commented 8 months ago

Yes, if you're willing, splitting into smaller patches would be great. Make it easy for me to hit that submit button confident there are no bugs :)

deepend-tildeclub commented 8 months ago

Yes, if you're willing, splitting into smaller patches would be great. Make it easy for me to hit that submit button confident there are no bugs :)

Yeah I understand. At least you got those missing files now. That helped a ton when I was trying to fix that section.

RedDragonWebDesign commented 8 months ago

Nice job finding the missing files. There was one version of these scripts in the official GitHub, and there is a second official GitHub, and there was a third version on the official download site. I think one of these versions was incomplete, or they diverged, or something. Tricky to reconcile!

deepend-tildeclub commented 8 months ago

Nice job finding the missing files. There was one version of these scripts in the official GitHub, and there is a second official GitHub, and there was a third version on the official download site. I think one of these versions was incomplete, or they diverged, or something. Tricky to reconcile!

sounds like one big mess. other then finding missing files which is nice, probably best to only focus on one and forget the rest exist lol

RedDragonWebDesign commented 8 months ago

One version was from 2014 and one version was from 2016. I used the 2016 version as the base for this repo. 602bc2d