Only prepend namespace for known classes #702

romaninsh · 2019-05-07T22:10:05Z

Currently atk4\ui\App normalize class will prepend \atk4\ui\ when using short name of the class, like this:

$app->add('CRUD');

# factory uses \atk4\ui\CRUD

However, this requires us to always use full name for the class:

$app->add('\my\test\MyClass')

This is incompatible with:

use \my\test\MyClass;

$app->add(MyClass::class);

because it uses my\test\MyClass without opening backslash.

This PR fixes problem by checking for the presence of the class inside \atk4\ui\ before attempting to use the prefix.

DarkSide666

Good addition, but tests now fail. Please take a look.

abbadon1334

if i remember correctly class and namespace checks (is_a, instanceof, class_exists) are all checked by advancing char position.

For performance is better not to recheck an already checked class?
i mean if the requested identifier was already requested answer directly with previously populated value of array.

Is a good feature but i suggest add a @todo can be improved caching results in array?

abbadon1334 · 2019-05-12T07:42:14Z

Is a good feature but i suggest add a @todo can be improved caching results in array?

nevermind, profiling the case it shows that is still checked by char like the other comparison class/interface functions, but cache is not needed.
Sorry but using socket i need to see any slowness and memory leak even if is relatively obviously that is managed at low level.

https://gist.github.com/88bc2fde67ee2e584559ae3ce0ccfe3d

abbadon1334 · 2019-05-12T08:00:31Z

src/App.php

@@ -376,7 +376,10 @@ public function addStyle($style)
     */
    public function normalizeClassNameApp($name)
    {
-        return '\\'.__NAMESPACE__.'\\'.$name;
+        if (class_exists('\\'.__NAMESPACE__.'\\'.$name)) {


NOTE: I think we need to talk about declaration of argument type :

in comments for static analysis - fast no control at runtime

in function declaration raise exception - "slow" strict check at runtime

related video of Rasmus Lerdorf - https://www.youtube.com/watch?v=rKXFgWP-2xQ

php 5.6 support ended on 01-01-2019 ( https://www.php.net/supported-versions.php )

/* can be defined as constant */ $ns_sepa = chr(92); /** test against untouched name */ if (class_exists($name)) { return $name; } /** normalize namespace */ $normalized_namespace = $ns_sepa.__NAMESPACE__; /** normalize name */ $normalized_name = $ns_sepa.trim($name,$ns_sepa); /** test against normalize name */ if (class_exists($normalized_name)) { return $normalized_name; } /** test against normalized namespace + normalize name */ if (class_exists($normalized_namespace.$normalized_name)) { return $normalized_namespace.$normalized_name; } /** last chance to check it */ /* give error even with this extra check $atk4_namespace = $ns_sepa.'atk4/ui'; if (class_exists($atk4_namespace.$normalized_name)) { return $atk4_namespace.$normalized_name; } */ /* it will throw exception for sure check to help sleepy dev */ // char(47) = / aka wrong slash used as separator if(strpos($name,chr(47)) !== false) { throw new Exception('Wrong separator used for class, detect / in place of \ called from class ' . $name); } /** class not exists DIE */ throw new Exception('Class with name ' . $name . ' not exists, checked ' . $normalized_name. ' | ' . $normalized_namespace . $normalized_name);

BUT even if in this way it works as intended, it redeclare classes and throw errors in some demos unittest.
Errors are due to misuse of wrong slash in place of backslash in $name.
We can make a str_replace and lose performance and solving a problem of the dev, in my opinion this is wrong, or throw an exception and make it changes the code or add a flag and make this function switchable in App.

i remember somewhere there was something on this subject of usage of slash in place of backslash

https://agile-core.readthedocs.io/en/latest/factory.html#namespace

we need to find where to put the str_replace of slash to backslash, before all or at the end and make another call to the function and finally throw exception?

My suggested version, it pass all tests :

public function normalizeClassNameApp($name) { /* can be defined as constant */ $ns_sepa = chr(92); /** test against untouched name */ if (class_exists($name)) { return $name; } /** normalize namespace */ $normalized_namespace = $ns_sepa.__NAMESPACE__; /** normalize name */ $normalized_name = $ns_sepa.trim($name,$ns_sepa); /** test against normalize name */ if (class_exists($normalized_name)) { return $normalized_name; } /** test against normalized namespace + normalize name */ if (class_exists($normalized_namespace.$normalized_name)) { return $normalized_namespace.$normalized_name; } /** last chance to check it */ /* give error even with this extra check $atk4_namespace = $ns_sepa.'atk4/ui'; if (class_exists($atk4_namespace.$normalized_name)) { return $atk4_namespace.$normalized_name; } */ /* it will throw exception for sure check to help sleepy dev */ // char(47) = / aka wrong slash used as separator if(strpos($name,chr(47)) !== false) { return $this->normalizeClassNameApp(str_replace(chr(47), chr(92), $name)); } /** class not exists DIE */ throw new Exception('Class with name ' . $name . ' not exists, checked ' . $normalized_name. ' | ' . $normalized_namespace . $normalized_name); }

gowrav-vishwakarma · 2019-05-13T08:27:44Z

Why not using it like

use \my\test\MyClass;
$app->add(new MyClass);

abbadon1334 · 2019-05-13T08:52:11Z

Why not using it like
use \my\test\MyClass;
$app->add(new MyClass);

IMHO because :

using new you make the object too coupled
using new you if you change the construct of that object you have to refactor in all project, opensource speaking all user of the project need to do that modification or put an argument optional which is not ok

i'm working on \atk4\core\ConfigTrait to be used like slim with callback in config to instantiate objects against interface implementation

like :

$return = [
    "logger.name" => "app",
    "logger.path" => "var/log/app-log.log",
    \Psr\Log\LoggerInterface::class => function($container) {
        $logger = new \Monolog\Logger($container->getConfig('logger.name'));
        /* init logger blah blah blah */
        return $logger;
    }
];

romaninsh · 2019-05-13T19:59:10Z

OK lets discuss on a hangout later.

romaninsh · 2019-05-23T18:29:47Z

will keep using "class_exist" here even if it has some performance hit.

this can solve every case

codecov · 2019-05-29T06:37:30Z

Codecov Report

Merging #702 into develop will decrease coverage by 2.75%.
The diff coverage is 71.42%.

@@             Coverage Diff              @@
##             develop    #702      +/-   ##
============================================
- Coverage      72.55%   69.8%   -2.76%     
- Complexity      1933    2356     +423     
============================================
  Files            105     106       +1     
  Lines           4559    5348     +789     
============================================
+ Hits            3308    3733     +425     
- Misses          1251    1615     +364

Impacted Files	Coverage Δ	Complexity Δ
src/App.php	`74.76% <71.42%> (+1.85%)`	`225 <4> (+108)`	⬆️
src/jsModal.php	`53.84% <0%> (-8.66%)`	`11% <0%> (+4%)`
src/CRUD.php	`42.85% <0%> (-5.15%)`	`79% <0%> (+25%)`
src/jsSearch.php	`91.42% <0%> (-2.33%)`	`9% <0%> (+5%)`
src/jsSSE.php	`16.98% <0%> (-0.33%)`	`21% <0%> (ø)`
src/FormField/Radio.php	`100% <0%> (ø)`	`18% <0%> (+2%)`	⬆️
src/FormField/MultiLine.php	`0% <0%> (ø)`	`110% <0%> (?)`
src/Card.php	`96.15% <0%> (+0.91%)`	`10% <0%> (+3%)`	⬆️
src/FormField/Generic.php	`96.55% <0%> (+1.09%)`	`13% <0%> (+2%)`	⬆️
src/TableColumn/Generic.php	`84.29% <0%> (+1.77%)`	`42% <0%> (+6%)`	⬆️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2c0b756...5af06eb. Read the comment docs.

codecov · 2019-05-29T06:37:31Z

Codecov Report

Merging #702 into develop will decrease coverage by 2.75%.
The diff coverage is 71.42%.

@@             Coverage Diff              @@
##             develop    #702      +/-   ##
============================================
- Coverage      72.55%   69.8%   -2.76%     
- Complexity      1933    2356     +423     
============================================
  Files            105     106       +1     
  Lines           4559    5348     +789     
============================================
+ Hits            3308    3733     +425     
- Misses          1251    1615     +364

Impacted Files	Coverage Δ	Complexity Δ
src/App.php	`74.76% <71.42%> (+1.85%)`	`225 <4> (+108)`	⬆️
src/jsModal.php	`53.84% <0%> (-8.66%)`	`11% <0%> (+4%)`
src/CRUD.php	`42.85% <0%> (-5.15%)`	`79% <0%> (+25%)`
src/jsSearch.php	`91.42% <0%> (-2.33%)`	`9% <0%> (+5%)`
src/jsSSE.php	`16.98% <0%> (-0.33%)`	`21% <0%> (ø)`
src/FormField/Radio.php	`100% <0%> (ø)`	`18% <0%> (+2%)`	⬆️
src/FormField/MultiLine.php	`0% <0%> (ø)`	`110% <0%> (?)`
src/Card.php	`96.15% <0%> (+0.91%)`	`10% <0%> (+3%)`	⬆️
src/FormField/Generic.php	`96.55% <0%> (+1.09%)`	`13% <0%> (+2%)`	⬆️
src/TableColumn/Generic.php	`84.29% <0%> (+1.77%)`	`42% <0%> (+6%)`	⬆️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2c0b756...5af06eb. Read the comment docs.

abbadon1334 · 2019-05-29T06:38:10Z

@romaninsh OK, i check the flow and i see that exception for class existence is throw in atk4/core/FactoryTrait

romaninsh · 2019-06-03T17:05:49Z

Looks like a better implementation is in #726. Perhaps I can split that out into a separate PR.

romaninsh · 2019-06-03T17:17:53Z

Oh you've done that for me!! yey.

romaninsh and others added 2 commits May 7, 2019 23:05

Only prepend class with "atk4\ui" if such class exist..

94ae37d

Apply fixes from StyleCI

99d3be2

romaninsh changed the title ~~Feature/only prepend namespace for known classes~~ Only prepend namespace for known classes May 7, 2019

romaninsh requested a review from DarkSide666 May 7, 2019 22:10

romaninsh added the in review label May 7, 2019

romaninsh self-assigned this May 7, 2019

romaninsh requested review from gowrav-vishwakarma and PhilippGrashoff May 7, 2019 22:21

DarkSide666 approved these changes May 8, 2019 •

edited

Loading

View reviewed changes

DarkSide666 self-requested a review May 8, 2019 12:06

DarkSide666 requested changes May 8, 2019

View reviewed changes

abbadon1334 reviewed May 11, 2019

View reviewed changes

abbadon1334 reviewed May 12, 2019

View reviewed changes

romaninsh added the hangout agenda 🔈 Will discuss on next hangout label May 13, 2019

romaninsh removed the hangout agenda 🔈 Will discuss on next hangout label May 23, 2019

abbadon1334 and others added 2 commits May 29, 2019 09:29

Update normalizeClassNameApp

ef7ae7f

this can solve every case

Apply fixes from StyleCI

5af06eb

abbadon1334 added a commit to abbadon1334/ui that referenced this pull request Jun 2, 2019

atk4#702 - NormalizeClassMap

e3dd741

abbadon1334 mentioned this pull request Jun 2, 2019

Some new TableColumns #726

Closed

2 tasks

romaninsh merged commit 51e3a01 into develop Jun 3, 2019

romaninsh deleted the feature/only-prepend-namespace-for-known-classes branch June 3, 2019 17:18

romaninsh mentioned this pull request Jun 12, 2019

Added ExternalLink, KeyValue, NoValue, Labels, Tooltip and ColorRating table decorators #743

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only prepend namespace for known classes #702

Only prepend namespace for known classes #702

romaninsh commented May 7, 2019

DarkSide666 left a comment

abbadon1334 left a comment

abbadon1334 commented May 12, 2019

abbadon1334 May 12, 2019 •

edited

Loading

abbadon1334 May 12, 2019

abbadon1334 May 13, 2019 •

edited

Loading

gowrav-vishwakarma commented May 13, 2019

abbadon1334 commented May 13, 2019 •

edited

Loading

romaninsh commented May 13, 2019

romaninsh commented May 23, 2019

codecov bot commented May 29, 2019

codecov bot commented May 29, 2019 •

edited

Loading

abbadon1334 commented May 29, 2019

romaninsh commented Jun 3, 2019

romaninsh commented Jun 3, 2019

Only prepend namespace for known classes #702

Only prepend namespace for known classes #702

Conversation

romaninsh commented May 7, 2019

DarkSide666 left a comment

Choose a reason for hiding this comment

abbadon1334 left a comment

Choose a reason for hiding this comment

abbadon1334 commented May 12, 2019

abbadon1334 May 12, 2019 • edited Loading

Choose a reason for hiding this comment

abbadon1334 May 12, 2019

Choose a reason for hiding this comment

abbadon1334 May 13, 2019 • edited Loading

Choose a reason for hiding this comment

gowrav-vishwakarma commented May 13, 2019

abbadon1334 commented May 13, 2019 • edited Loading

romaninsh commented May 13, 2019

romaninsh commented May 23, 2019

codecov bot commented May 29, 2019

Codecov Report

codecov bot commented May 29, 2019 • edited Loading

Codecov Report

abbadon1334 commented May 29, 2019

romaninsh commented Jun 3, 2019

romaninsh commented Jun 3, 2019

abbadon1334 May 12, 2019 •

edited

Loading

abbadon1334 May 13, 2019 •

edited

Loading

abbadon1334 commented May 13, 2019 •

edited

Loading

codecov bot commented May 29, 2019 •

edited

Loading