Loader fixes and improvements #1769

Closed
wants to merge 5 commits into
from

Projects

None yet

5 participants

@nickl-
nickl- commented Sep 4, 2012

Also fixes issues with CI_Session prefixed subclass extending identified at #353

Includes unit tests.

See commit messages and diffs for details.

nickl- added some commits Sep 4, 2012
@nickl- nickl- No need to track loaded files.
PHP already tracks all included files, through the mechanisms employed by the *_once include/require methods. The collection of all included files can be retrieved through get_included_files().
Removed the local property _ci_loaded_files and the implementations where newly included files are added to the collection.
Replaced the enquiring of included files with the built in PHP functionality.
b9effd0
@nickl- nickl- Unit tests load and subclass Session #353.
Unit tests to reproduce the problem raised in #353.
In addition to testing load by libary or load by driver we are also extending by prefixed sub-class.
Added new mack application folder.
Added Session Subclass mock.
Fix bootstrap to point APPPATH to the mock application folder.
Including IC_Session class more than once in the same unit test requires runInSeparateProcess and preserveGlobalState disabled
which also causes all knowledge of bootstrap to dissapear so we need to cater for that.
7102c63
@nickl- nickl- Suppress headers already sent warnings. 63bf687
@nickl- nickl- Fix #353 Prefixed subclass Session extended.
Support for case sensitive FS
Allows for extended class to be in library or library/Session folders.
Solves problems extending Session as identified in #353.
22e3549
@nickl- nickl- Fix #353 Allow Session extend with or without driver.
To extend the Session and drivers you would add same named drivers to the valid_drivers collection.
This allows you to still fall back to the standard session drivers eves thaugh we extended CI_Session.
Also helps solving problems discovered in #353.
898805b
@nickl-
nickl- commented Sep 11, 2012

From #353 and in agreement wit @narfbg that discussions should not be on closed issues.

I was wondering the same thing, why are we discussing these issues on a closed thread but when in Rome, thought this was the way. See that changed now and my +1 as well.

Herewith @narfbg post on #353 for convenience

@nickl- A quick glance at your and @dchill42's comments here shows that you're mostly discussing other pull requests here, which is hard to follow (consider this a suggestion to move the discussion :)) and the only new thing I can comment on is again +1 for @dchill42 - putting @ in front of setcookie() is pointless and only causes a few more CPU cycles to be wasted.

If by added CPU cycles you mean the fact that the app doesn't crash and actually continues, yes those would be considered a lot more cycles but arguably the ones we actually want. Other than that the @ only suppresses errors which only comes into play when an error actually occurred so no extra work for your cpu and not the same then as if (headers_sent()) in that regard.

That said I am easy, I just like adding the @ to header and set_cookie because it simplifies debugging since the most likely time these errors are raised is because you are trying to respond out of turn and it doesn't help if the app dies on you because you do.

Otherwise, to be honest - I don't get why is there discussion here on file paths at all. I'd like to get more info on those special caching provisions that you're talking about, but as I said - we need to track loaded libraries and class names, not file paths.

Where do we pick these things up? I see Luke Scott also mentions it in the manual but he also doesn't exactly answer your question.

It might stem from discussions on the internals mailing list or responses to RFCs like rfc:auto_include I will have to look around to see if I can find something more precise.

@narfbg
Contributor
narfbg commented Sep 11, 2012

setcookie() doesn't trigger any warnings or notices, so there's nothing to suppress.

@nickl-
nickl- commented Sep 11, 2012

That is strange because when I do:

<h4>I wonder what the documentation means when they say:</h4>
<em> Like other headers, cookies must be sent before any output from your script?</em><br>
Lets see:

<?php
setcookie(
    "ErrorCookie", 
    "setcookie() doesn't trigger any warnings or notices."
);

I get:

I wonder what the documentation means when they say:

Like other headers, cookies must be sent before any output from your script?
Lets see:

PHP Warning: Cannot modify header information - headers already sent by (output started at test.php:5) in test.php on line 7

You don't get that?

@alexbilbie
Contributor

@nicki well that would make sense because you're outputting content and then sending a header. Try setting the cookie then outputting the content

@narfbg
Contributor
narfbg commented Sep 11, 2012

@nickl- No - I don't use setcookie() after other output. ;)

Error messages are useful - they must not be suppressed unless we know that they will be triggered and that behavior is indended.

@dchill42
Contributor

I'm working up a parallel request that should satisfy the agreed issues here, without changing any of the code that should be left alone. I'll post it and link here as soon as it's up.

@dchill42
Contributor

I still need to add unit testing to cover the changes (and Cache doesn't even have unit tests yet), but a draft of the code is available for review and testing. Existing (relevant) unit tests pass.

@narfbg narfbg referenced this pull request Nov 26, 2012
Closed

Cleaned up class loader #2031

@narfbg
Contributor
narfbg commented Jan 28, 2013

@nickl- Could you update this PR please? Some of the changes in it have already been made and I'm looking forward to implementing the rest of your improvements, but it has become outdated ...

@nickl-
nickl- commented Jan 28, 2013

@narfbg That is awesome news! I was under the impression a consensus was reached that these were not of any use. If you are keen however I will gladly rebase against current head.

Which features in particular are you considering?
I will likely require about a week to get around to this, unfortunately in this time (albeit relatively short) I was the lucky recipient of a HDD failure and will have to either retrieve this from archive or start from scratch. None of which should raise any concerns, it only means that it's not just a quick rebase/merge and submit.

@narfbg
Contributor
narfbg commented Jan 28, 2013

Unit tests in particular - those are not really a part of my expertise and you seem to be motivated to improve them. :) Also, due to my recent changes they appear to be broken due to some dependancy on the tests' autoloader - even fixing that would be enough.

@nickl-
nickl- commented Jan 29, 2013

@narfbg No problem...

@nickl-
nickl- commented Mar 26, 2013

It would seem these slipped through the cracks somehow... brb

@TheDigitalOrchard

Update?

@nickl-
nickl- commented May 1, 2013

Still haven't found the time... what is the roadmap looking like, have you considered a release date yet? Perhaps some urgency will help avoid distractions. Lets see what we can do quick.

Nope not straight forward need more time...

@narfbg
Contributor
narfbg commented Sep 23, 2013

Due to many changes in the Loader you'd have to rewrite/resubmit this one, if there's anything left to improve. :)

@narfbg narfbg closed this Sep 23, 2013
@nickl-
nickl- commented Nov 28, 2013

fare-e-nuff =)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment