Skip to content
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

#820 #818 #823 #821 #825 #826 #839 #822 are fixed #841

Merged
merged 47 commits into from
Dec 21, 2022
Merged

#820 #818 #823 #821 #825 #826 #839 #822 are fixed #841

merged 47 commits into from
Dec 21, 2022

Conversation

belisoful
Copy link
Member

#820 caches all fx methods in performance mode.
#818 removed phpdoc build up of "dy" methods from TCallChain.
#823 set TBehavior Enabled default to true, removed the setEnabled on initialize.
#821 checks for the property/event in the behavior before checking its isset().
#825 Adds TComponent::hasMethod replacement for method_exists that is behavior aware..
#826 Adds TComponent::getBehaviors.
#839 correctly detaches a behavior in the reverse order as attach.
#822 returns IClassBehavior or array of attached IBehavior[s] from TComponent::*tachClassBehavior.

with appropriate unit tests.

These can all be closed upon merge at this point.

There are some documentation updates as well.

I'm just going through the bugs, and thoroughly correcting and testing them. So there will be more commits to fix more bugs.

However... Given the dependency of #836 upon these bug fixes, it seemed prudent to merge these now.

Also, I want the merge to be manageable in size. This is already big enough.

The table widget will incorrectly wrap the last column-row with many long columns due to not resetting on each row.
If there is an error in a test, the uncleared tasks could upset future tasks.
…error

This initializes new cron tasks' lastExecTime in setPersistentData.  lastExecTime initializes in DBCron tasks on add or change of schedule on update.  This corrects the bug of triggering on first run when it shouldn't.  Continually running after trigger for one off tasks is also stopped with better null handling.
Updated some documentation.
tweaked some validation to be more efficient, eg TTimeScheduler
Removed timezone corrections from getNextTriggerTime.
Cron Shell shows when tasks haven't run.
Added a high efficiency "@(\d)" TimeStamp-based Scheduler format for one off tasks.  eg "@1668165071". significantly reduces validation, parse, and nextTriggerTime computations.
Updated unit tests for updated lastExecTime on tasks, etc.
A cron run should only be running tasks within it's own minute.
…uting in current minute" logic

Basically this is required because we know each minute cron is running.  If a tasks takes longer than the current executing minute, the further tasks should not be executed due to being picked up by the next crontab prado cron.

This preserves the CLI functionality that when PRADO cron is manually run from the CLI, all pending tasks are completed regardless of when its run or how long it takes.
…d moved the filterStaleTasks early

This avoids global state overwrites by multiple cron minutes that was causing multiple erroneous runs.  TDbCronModule needs to ensure all tasks are loaded to filter.  such heavy processing is corrected in version 2 of T*Cron.
Is there a better way to include all "dy*" @method?
Corrected with DIRECTORY_SEPARATOR
It only looks for the $_ENV['TERM'] and even if there, when it is 'unknown' then we presume to be in cron tab environment combined with sudo.
TCronModule gets property InCronShell configured from a CLI option.  "--cron", as a CLI option, configures cron to only run tasks during the current minute and leaves anything beyond that to the next cron process.  This prevents long running tasks from running other later pending tasks multiple times.  Linux has automated cron shell detection when '--cron' is not set but may not work in Windows.

Update TShellApplication to have descriptions and values for the option to be displayed on help cli command.  TShellLoginBehavior adds descriptions and default values to their CLI options.
Removed logging the cron and no filtering the tasks.  But we should disable shell logging behavior.
Minor tweak, squeeze that performance out.
stating that defaults can be set with Skins.
All the other locations are definitional rather than for PHPStan conformance.
When TBehaviorsModule loads behaviors, they can be Enable="false" to have them be turned off
This is a slighting bigger bug fix than initially formulated.

All PRADO behaviors have been reviewed for proper disabled behavior with this commit.  TPermissionsBehavior should always load the data even when disabled (which shouldn't happen in typical conditions anyway)
Permissions Tests needed to unlisten to ensure test compliance
    and
 Time Schedule tests wasn't properly testing null cases
This encodes that only one layer of behaviors is officially supported.  Behaviors in behaviors is an experimental use and can be done and explored.

This also corrects one test and adds a few more tests for enabling and disabling behaviors.
This has been needed for a while.
This gets the behavior properly at the priority and detaches in the opposite order of attaching.
… being *tached

The return result of *tachClassBehavior is document and return values unit tested.
with unit tests for the bug.  It failed with the original code, and passes with the new code.
behavior names are unique to the owner object.   But To reference behaviors to attach second layer behaviors, the behavior names must be unique across the configured TBehaviorsModule behaviors.  Multiple TBehaviorsModules (eg. a second one in TPageService for page specific behaviors) can refer to the behaviors of the first TBehaviorsModule.

With unit Tests
@ctrlaltca
Copy link
Member

I'm sorry, but this is quite impossible to review.
Each fix should go in a separate PR.
Feel free to merge anyway if you are confident.

@belisoful
Copy link
Member Author

belisoful commented Dec 21, 2022

@ctrlaltca My humble apologies. I tried keeping each bug fix to one commit for that reason. there are a slew of minor tweaks and bug fixes that would take a while going one by one. I have been keeping the unit tests up to date and triple checked. I wanted to get through a few of the bugs at once because of the dependency of the fixes from #836.

I haven't deviated from any stated intentions or code discussions/agreement.

I am very confident of this merge to this point, especially given the degree of time I've spent on the unit tests. I will take responsibility for any issues of this merge, though I am confident there shouldn't be any. Especially given the thoroughness of the unit tests.

From here on, I'll keep to your advice. Also, thank you for your own confidence in me, these minor tweaks, and the unit tests.

I'll see if I can reset my branch after this merge so all the prior accepted commits are cleaned for further PRs.

@belisoful
Copy link
Member Author

belisoful commented Dec 21, 2022

Many of these bugs are from my own improper coding. I want to get them corrected ASAP. Doing one final scrub to $this->ensureCode();

@belisoful
Copy link
Member Author

40% of the merge is new unit tests to ensure that the code update is properly functional. 10% is documentation update. FYI.

@belisoful belisoful merged commit 1697042 into pradosoft:master Dec 21, 2022
@belisoful
Copy link
Member Author

@ctrlaltca If you want me to close the issues, I can, but if you want to verify issue each before closing as a way to check the work, I understand. Having written the unit tests and triple checked them, I'll tell you that the unit tests are solid. IF you don't want to spend the time with it, let me know, I'll close the issues.

The other issues are more than just basic tweaks and why I think they definitely should be their own PR each. I put them off for that reason and only focused on these [systemic-oriented patches].

Again, my humble apologies for making an unwieldy merge of minor bug fixes. I got to working.... and working and working.

@belisoful
Copy link
Member Author

I was hoping that keeping (for the most part) one commit per issue would keep things at least somewhat organized, no?

@ctrlaltca
Copy link
Member

Don't worry, that's fine. I see the commits are well separated in the commit history, that's good for future troubleshooting.
Go on closing the issue, unfortunately I'm really short on time in these weeks; thank you for taking care of it.

@belisoful
Copy link
Member Author

Enjoy the holidays.

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