Fix for issue #35. Corrected ActiveRecordTest's so that capitalizatio…

[ipundit]

Fix for issue #35. Corrected ActiveRecordTest's so that capitalization for attribute names are retained. So the short form for New York is now NY instead of ny. test/model/Author, Book, and Venue now have those strtoupper() workarounds in getter/setter methods removed as it's done in Model.php

[shmax]

Shouldn't this be something like:

$methodName = "get_" . strtolower($name) ;
if (method_exists($methodName) {
            $retValue = $this->$name();
            return $retValue;

...that way you don't have to worry about stuff breaking if you make it down to line 358, right? (maybe we should add tests around non-lowercase attribute names?)

[shmax]

Also, fun TODO: make the casing configurable (eg. I use camelCase in my own codebase, not snake_case).

Edit: when I say TODO I mean for some far-off day, not this PR!

[shmax]

Not sure what happened, but can you see if you can rebase so only your changes remain in this PR?

[ipundit]

Not sure what happened, but can you see if you can rebase so only your changes remain in this PR?

Is this ok now? I have a PR for issue #37 that I want to push. It may also affect PR #38

[shmax]

Yep, much better, but could you address the comment I left?

[ipundit]

I thought I did about method_exists(). If it's about camel case vs snake case configuration, I think what you're asking would lead to a bug. SQL distinguishes between attributeName and attribute_name as two different columns in a table; it's only the case that is insensitive, not the camel vs snake spelling. Collapsing them into one column though a configuration would prevent the use of ActiveRecord in the case where a table has attributeName and attribute_name as columns. Not that I would recommend anyone designing a table that way, but if they did, it should be treated as two columns, not one through a configuration.

[shmax]

Ah, no, actually I was just talking about the style of the magic set methods.

In my codebase I use camelCase for everything, but because ActiveRecord assumes snake_case, I have this jarring mix of styles on my methods:

class Animal extends Model {
  function forageForFood() {
     // do stuff
  }

  public function set_name(string $name): void
  {
      $this->assign_attribute('name', $name);
      $this->forageForFood();
   }
}

But that whole concept may or may not make sense, and is a topic for another day. The comment I was referring to was this:
#36 (comment)

[ipundit]

Shouldn't this be something like:

$methodName = "get_" . strtolower($name) ;
if (method_exists($methodName) {
            $retValue = $this->$name(); // $name must be lowercase because $methodName is lowercase and you're calling $methodName
            return $retValue;

...that way you don't have to worry about stuff breaking if you make it down to line 358, right? (maybe we should add tests around non-lowercase attribute names?)

As the comment above, $this->$name() should be calling the lowercase version of $name as $methodName is lowercase. My original code does this and passes regression.

The PR allows you to do $model->attrName or $model->attrname or $model->ATTRNAME. Getting and setting attributes are no longer case sensitive, just like the generated SQL.

[shmax]

So, I'm still trying to wrap my head around what it is you're trying to fix, here, but my first thought is that you may be misunderstanding what's happening in some of the tests. Let's start with this one:

    public function test_getter()
    {
        $book = Book::first();
        $this->assertEquals(strtoupper($book->name), $book->upper_name);
    }

This isn't a workaround for anything. What it's doing is confirming that the custom getter is working, specifically this one:

// Book.php
public function get_upper_name()
    {
        return strtoupper($this->name);
    }

Is that clear? We're just testing that the custom getter works. The uppercase thing is just a way to transform the value in a way that's easy to verify in our test. We could have just as easily reversed the string, or hashed it, or whatever. It has nothing to do with the internal handling of casing of column names.

All that said, that test is sort of breaking one of the laws of testing, which is not to do any business logic in the test, so it should probably be rewritten as:

    public function test_getter()
    {
        $book = Book::first();
        $this->assertEquals("THE ANCIENT ART OF MAIN TANKING", $book->upper_name);
    }

[shmax]

not to do any business logic in the test

Fixed here:
70470a4#diff-cf8b9df6c71a05581588d7224115451eb753219df9eaa94db89a188095610c47R456

[ipundit]

Hopefully, the new tests of 80ed052 illustrate the case insensitivity fix for issue #35 now

[shmax]

Okay, let me see if I can summarize the situation, here:

1. Let's say we have a table with a column name that uses odd casing, such as Author_Id.
2. after the table metadata loads, the name eventually passes through an inflection step as it transfers to Model::$attributes, becoming author_id
3. If client code tries to access the field with its correct casing in good faith, like this...

$author = Author::find(1);
$authorId = $author->Author_Id; // exception

...then it kerplodes.

Do I have that right?

[ipundit]

Yes. And for those of use using camel case, $model->longvariablenamewithoutcamelcaseishardtoread == true!

[shmax]

Okay, then let's proceed, but can you undo the changes to the singular/array/null tests? If you want to keep those tests, then please move them to your test_case_sensitivity test.

[ipundit]

Yes, I can do that, but need clarity as to which is the minimum set of tests you want to keep. Do you want any of the tests in ActiveRecordFindTest.php? They are more documentation purposes than anything else, and perhaps it should just be stated that attribute names are case insensitive in the documentation.

Do you want any tests outside of test_case_insensitivity()? None of the rest are necessary.

[shmax]

Do you want any tests outside of test_case_insensitivity()? None of the rest are necessary.

Let's start by keeping the ones in test_case_insensitivity and revert the rest. We can always add more as needed.

[ipundit]

Ok, reverted. Fixed readme.md to have lists in alphabetical order and for contributing.md to point to the the right file.

[shmax]

Awesome!