Attribute cleanup and optimisation in preparation to support attribute tags, HomeController return casting

app/Classes/LDAP/Attribute.php

@@ -17,31 +17,27 @@ class Attribute implements \Countable, \ArrayAccess, \Iterator
 	protected string $name;
 	private int $counter = 0;
 
-	protected ?AttributeType $schema = NULL;
-
-	/*
-	# Source of this attribute definition
-	protected $source;
-	*/
-
-	// Current and Old Values
-	protected Collection $values;
-
 	// Is this attribute an internal attribute
-	protected bool $is_internal = FALSE;
+	protected(set) bool $is_internal = FALSE;
 
 	// Is this attribute the RDN?
-	protected bool $is_rdn = FALSE;
+	public bool $is_rdn = FALSE;
 
 	// MIN/MAX number of values
-	protected int $min_values_count = 0;
-	protected int $max_values_count = 0;
+	protected(set) int $min_values_count = 0;
+	protected(set) int $max_values_count = 0;
 
 	// RFC3866 Language Tags
+	/* @deprecated use $values/$values_old when playing with language tags */
 	protected Collection $lang_tags;
 
+	// The schema's representation of this attribute
+	protected(set) ?AttributeType $schema;
+
 	// The old values for this attribute - helps with isDirty() to determine if there is an update pending
-	protected Collection $oldValues;
+	protected(set) Collection $values_old;
+	// Current Values
+	public Collection $values;
 
 	/*
 	# Has the attribute been modified
@@ -97,9 +93,10 @@ class Attribute implements \Countable, \ArrayAccess, \Iterator
 	public function __construct(string $name,array $values)
 	{
 		$this->name = $name;
-		$this->values = collect($values);
+		$this->values_old = collect($values);
+
+		$this->values = collect();
 		$this->lang_tags = collect();
-		$this->oldValues = collect($values);
 
 		$this->schema = (new Server)
 			->schema('attributetypes',$name);
@@ -132,18 +129,10 @@ class Attribute implements \Countable, \ArrayAccess, \Iterator
 			'hints' => $this->hints(),
 			// Can this attribute be edited
 			'is_editable' => $this->schema ? $this->schema->{$key} : NULL,
-			// Is this an internal attribute
-			'is_internal' => isset($this->{$key}) && $this->{$key},
-			// Is this attribute the RDN
-			'is_rdn' => $this->is_rdn,
 			// We prefer the name as per the schema if it exists
 			'name' => $this->schema ? $this->schema->{$key} : $this->{$key},
 			// Attribute name in lower case
 			'name_lc' => strtolower($this->name),
-			// Old Values
-			'old_values' => $this->oldValues,
-			// Attribute values
-			'values' => $this->values,
 			// Required by Object Classes
 			'required_by' => $this->schema?->required_by_object_classes ?: collect(),
 			// Used in Object Classes
@@ -153,17 +142,6 @@ class Attribute implements \Countable, \ArrayAccess, \Iterator
 		};
 	}
 
-	public function __set(string $key,mixed $values): void
-	{
-		switch ($key) {
-			case 'value':
-				$this->values = collect($values);
-				break;
-
-			default:
-		}
-	}
-
 	public function __toString(): string
 	{
 		return $this->name;
@@ -260,13 +238,8 @@ class Attribute implements \Countable, \ArrayAccess, \Iterator
 	 */
 	public function isDirty(): bool
 	{
-		return ($this->oldValues->count() !== $this->values->count())
-			|| ($this->values->diff($this->oldValues)->count() !== 0);
-	}
-
-	public function oldValues(array $array): void
-	{
-		$this->oldValues = collect($array);
+		return ($this->values_old->count() !== $this->values->count())
+			|| ($this->values->diff($this->values_old)->count() !== 0);
 	}
 
 	/**
@@ -292,7 +265,7 @@ class Attribute implements \Countable, \ArrayAccess, \Iterator
 
 	public function render_item_old(int $key): ?string
 	{
-		return Arr::get($this->old_values,$key);
+		return Arr::get($this->values_old,$key);
 	}
 
 	public function render_item_new(int $key): ?string
@@ -306,14 +279,10 @@ class Attribute implements \Countable, \ArrayAccess, \Iterator
 	 * @param string $tag
 	 * @param array $value
 	 * @return void
+	 * @deprecated
 	 */
 	public function setLangTag(string $tag,array $value): void
 	{
 		$this->lang_tags->put($tag,$value);
 	}
-
-	public function setRDN(): void
-	{
-		$this->is_rdn = TRUE;
-	}
 }

app/Classes/LDAP/Attribute/Binary.php

@@ -7,6 +7,6 @@ use App\Classes\LDAP\Attribute;
 /**
  * Represents an attribute whose values are binary
  */
-class Binary extends Attribute
+abstract class Binary extends Attribute
 {
 }

app/Classes/LDAP/Attribute/Internal.php

@@ -11,7 +11,7 @@ use App\Classes\LDAP\Attribute;
  */
 abstract class Internal extends Attribute
 {
-	protected bool $is_internal = TRUE;
+	protected(set) bool $is_internal = TRUE;
 
 	public function render(bool $edit=FALSE,bool $old=FALSE,bool $new=FALSE): View
 	{

app/Classes/LDAP/Attribute/KrbPrincipalKey.php

@@ -26,7 +26,7 @@ final class KrbPrincipalKey extends Attribute
 
 	public function render_item_old(int $key): ?string
 	{
-		$pw = Arr::get($this->oldValues,$key);
+		$pw = Arr::get($this->values_old,$key);
 		return $pw
 			? str_repeat('*',16)
 			: NULL;

app/Classes/LDAP/Attribute/KrbTicketFlags.php

@@ -11,7 +11,7 @@ use Illuminate\Support\Collection;
  * Represents an attribute whose value is a Kerberos Ticket Flag
  * See RFC4120
  */
-class KrbTicketFlags extends Attribute
+final class KrbTicketFlags extends Attribute
 {
 	private const DISALLOW_POSTDATED	= 0x00000001;
 	private const DISALLOW_FORWARDABLE	= 0x00000002;

app/Classes/LDAP/Attribute/Password.php

@@ -87,7 +87,7 @@ final class Password extends Attribute
 
 	public function render_item_old(int $key): ?string
 	{
-		$pw = Arr::get($this->oldValues,$key);
+		$pw = Arr::get($this->values_old,$key);
 		return $pw
 			? (((($x=$this->hash($pw)) && ($x::id() !== '*clear*')) ? sprintf('{%s}',$x::shortid()) : '').str_repeat('*',16))
 			: NULL;

app/Http/Controllers/HomeController.php

@@ -2,7 +2,6 @@
 
 namespace App\Http\Controllers;
 
-use Illuminate\Contracts\View\View;
 use Illuminate\Http\Request;
 use Illuminate\Support\Arr;
 use Illuminate\Support\Collection;
@@ -44,8 +43,6 @@ class HomeController extends Controller
 
 	/**
 	 * Debug Page
-	 *
-	 * @return \Illuminate\Contracts\Foundation\Application|\Illuminate\Contracts\View\Factory|\Illuminate\Contracts\View\View
 	 */
 	public function debug()
 	{
@@ -56,10 +53,10 @@ class HomeController extends Controller
 	 * Create a new object in the LDAP server
 	 *
 	 * @param EntryAddRequest $request
-	 * @return \Illuminate\Contracts\Foundation\Application|\Illuminate\Contracts\View\Factory|\Illuminate\Contracts\View\View
+	 * @return View
 	 * @throws InvalidUsage
 	 */
-	public function entry_add(EntryAddRequest $request)
+	public function entry_add(EntryAddRequest $request): \Illuminate\View\View
 	{
 		if (! old('step',$request->validated('step')))
 			abort(404);
@@ -92,15 +89,15 @@ class HomeController extends Controller
 	 *
 	 * @param Request $request
 	 * @param string $id
-	 * @return \Closure|\Illuminate\Contracts\View\View|string
+	 * @return \Illuminate\View\View
 	 */
-	public function entry_attr_add(Request $request,string $id): string
+	public function entry_attr_add(Request $request,string $id): \Illuminate\View\View
 	{
 		$xx = new \stdClass;
 		$xx->index = 0;
 
 		$x = $request->noheader
-			? (string)view(sprintf('components.attribute.widget.%s',$id))
+			? view(sprintf('components.attribute.widget.%s',$id))
 				->with('o',Factory::create($id,[]))
 				->with('value',$request->value)
 				->with('loop',$xx)
@@ -109,7 +106,7 @@ class HomeController extends Controller
 		return $x;
 	}
 
-	public function entry_create(EntryAddRequest $request)
+	public function entry_create(EntryAddRequest $request): \Illuminate\Http\RedirectResponse
 	{
 		$key = $this->request_key($request,collect(old()));
 
@@ -156,7 +153,7 @@ class HomeController extends Controller
 			->withFragment($o->getDNSecure());
 	}
 
-	public function entry_delete(Request $request)
+	public function entry_delete(Request $request): \Illuminate\Http\RedirectResponse
 	{
 		$dn = Crypt::decryptString($request->dn);
 
@@ -196,7 +193,7 @@ class HomeController extends Controller
 			->with('success',[sprintf('%s: %s',__('Deleted'),$dn)]);
 	}
 
-	public function entry_export(Request $request,string $id)
+	public function entry_export(Request $request,string $id): \Illuminate\View\View
 	{
 		$dn = Crypt::decryptString($id);
 
@@ -215,10 +212,10 @@ class HomeController extends Controller
 	/**
 	 * Render an available list of objectclasses for an Entry
 	 *
-	 * @param string $id
-	 * @return mixed
+	 * @param Request $request
+	 * @return Collection
 	 */
-	public function entry_objectclass_add(Request $request)
+	public function entry_objectclass_add(Request $request): Collection
 	{
 		$oc = Factory::create('objectclass',$request->oc);
 
@@ -242,7 +239,7 @@ class HomeController extends Controller
 				]);
 	}
 
-	public function entry_password_check(Request $request)
+	public function entry_password_check(Request $request): Collection
 	{
 		$dn = Crypt::decryptString($request->dn);
 		$o = config('server')->fetch($dn);
@@ -265,10 +262,10 @@ class HomeController extends Controller
 	 * Show a confirmation to update a DN
 	 *
 	 * @param EntryRequest $request
-	 * @return \Illuminate\Contracts\Foundation\Application|\Illuminate\Contracts\View\Factory|\Illuminate\Contracts\View\View|\Illuminate\Foundation\Application|\Illuminate\Http\RedirectResponse
+	 * @return \Illuminate\Http\RedirectResponse|\Illuminate\View\View
 	 * @throws ObjectNotFoundException
 	 */
-	public function entry_pending_update(EntryRequest $request)
+	public function entry_pending_update(EntryRequest $request): \Illuminate\Http\RedirectResponse|\Illuminate\View\View
 	{
 		$dn = Crypt::decryptString($request->dn);
 
@@ -277,6 +274,8 @@ class HomeController extends Controller
 		foreach ($request->except(['_token','dn','userpassword_hash','userpassword']) as $key => $value)
 			$o->{$key} = array_filter($value,fn($item)=>! is_null($item));
 
+		// @todo Need to handle incoming attributes that were modified by MD5Updates Trait (eg: jpegphoto)
+
 		// We need to process and encrypt the password
 		if ($request->userpassword) {
 			$passwords = [];
@@ -312,8 +311,10 @@ class HomeController extends Controller
 	 * @param EntryRequest $request
 	 * @return \Illuminate\Http\RedirectResponse
 	 * @throws ObjectNotFoundException
+	 * @todo When removing an attribute value, from a multi-value attribute, we have a ghost record showing after the update
+	 * @todo Need to check when removing a single attribute value, do we have a ghost as well? Might be because we are redirecting with input?
 	 */
-	public function entry_update(EntryRequest $request)
+	public function entry_update(EntryRequest $request): \Illuminate\Http\RedirectResponse
 	{
 		$dn = Crypt::decryptString($request->dn);
 
@@ -368,9 +369,9 @@ class HomeController extends Controller
 	 *
 	 * @param Request $request
 	 * @param Collection|null $old
-	 * @return \Illuminate\Contracts\Foundation\Application|\Illuminate\Contracts\View\Factory|View
+	 * @return \Illuminate\View\View
 	 */
-	public function frame(Request $request,?Collection $old=NULL): View
+	public function frame(Request $request,?Collection $old=NULL): \Illuminate\View\View
 	{
 		// If our index was not render from a root url, then redirect to it
 		if (($request->root().'/' !== url()->previous()) && $request->method() === 'POST')
@@ -385,7 +386,9 @@ class HomeController extends Controller
 
 		// If we are rendering a DN, rebuild our object
 		$o = config('server')->fetch($key['dn']);
-		foreach (collect(old())->except(['dn','_token']) as $attr => $value)
+
+		// @todo We need to dynamically exclude request items, so we dont need to add them here
+		foreach (collect(old())->except(['dn','_token','userpassword_hash']) as $attr => $value)
 			$o->{$attr} = $value;
 
 		return match ($key['cmd']) {
@@ -407,7 +410,7 @@ class HomeController extends Controller
 	/**
 	 * This is the main page render function
 	 */
-	public function home(Request $request)
+	public function home(Request $request): \Illuminate\View\View
 	{
 		// Did we come here as a result of a redirect
 		return count(old())
@@ -421,11 +424,11 @@ class HomeController extends Controller
 	 *
 	 * @param ImportRequest $request
 	 * @param string $type
-	 * @return \Illuminate\Contracts\Foundation\Application|\Illuminate\Contracts\View\Factory|\Illuminate\Contracts\View\View|\Illuminate\Foundation\Application
+	 * @return \Illuminate\View\View
 	 * @throws GeneralException
 	 * @throws VersionException
 	 */
-	public function import(ImportRequest $request,string $type)
+	public function import(ImportRequest $request,string $type): \Illuminate\View\View
 	{
 		switch ($type) {
 			case 'ldif':
@@ -461,9 +464,9 @@ class HomeController extends Controller
 	/**
 	 * LDAP Server INFO
 	 *
-	 * @return \Illuminate\Contracts\Foundation\Application|\Illuminate\Contracts\View\Factory|\Illuminate\Contracts\View\View
+	 * @return \Illuminate\View\View
 	 */
-	public function info()
+	public function info(): \Illuminate\View\View
 	{
 		return view('frames.info')
 			->with('s',config('server'));
@@ -507,10 +510,10 @@ class HomeController extends Controller
 	 *
 	 * @note Our route will validate that types are valid.
 	 * @param Request $request
-	 * @return \Illuminate\Contracts\Foundation\Application|\Illuminate\Contracts\View\Factory|\Illuminate\Contracts\View\View
+	 * @return \Illuminate\View\View
 	 * @throws InvalidUsage
 	 */
-	public function schema_frame(Request $request)
+	public function schema_frame(Request $request): \Illuminate\View\View
 	{
 		// If an invalid key, we'll 404
 		if ($request->type && $request->key && (! config('server')->schema($request->type)->has($request->key)))
@@ -536,9 +539,9 @@ class HomeController extends Controller
 	 * Return the image for the logged in user or anonymous
 	 *
 	 * @param Request $request
-	 * @return mixed
+	 * @return \Illuminate\Http\Response
 	 */
-	public function user_image(Request $request)
+	public function user_image(Request $request): \Illuminate\Http\Response
 	{
 		$image = NULL;
 		$content = NULL;
@@ -556,4 +559,4 @@ class HomeController extends Controller
 		return response($image)
 			->header('Content-Type',$content);
 	}
-}
+}

app/Ldap/Entry.php

@@ -97,7 +97,7 @@ class Entry extends Model
 		if ((! $this->objects->get($key)) && $value)
 			$this->objects->put($key,Factory::create($key,[]));
 
-		$this->objects->get($key)->value = $this->attributes[$key];
+		$this->objects->get($key)->values = collect($this->attributes[$key]);
 
 		return $this;
 	}
@@ -155,7 +155,10 @@ class Entry extends Model
 			$x->addValue($value);
 
 		} else {
-			$this->objects->put($key,Attribute\Factory::create($key,Arr::wrap($value)));
+			$o = Attribute\Factory::create($key,[]);
+			$o->values = collect(Arr::wrap($value));
+
+			$this->objects->put($key,$o);
 		}
 	}
 
@@ -164,31 +167,30 @@ class Entry extends Model
 	 *
 	 * @return Collection
 	 */
-	public function getAttributesAsObjects(): Collection
+	private function getAttributesAsObjects(): Collection
 	{
 		$result = collect();
 
-		foreach ($this->attributes as $attribute => $value) {
-			// If the attribute name has language tags
+		foreach ($this->attributes as $attribute => $values) {
+			// If the attribute name has tags
 			$matches = [];
 			if (preg_match('/^([a-zA-Z]+)(;([a-zA-Z-;]+))+/',$attribute,$matches)) {
 				$attribute = $matches[1];
 
 				// If the attribute doesnt exist we'll create it
-				$o = Arr::get($result,$attribute,Factory::create($attribute,[]));
-				$o->setLangTag($matches[3],$value);
+				$o = Arr::get($result,$attribute,Factory::create($attribute,Arr::get($this->original,$attribute,[])));
+				$o->setLangTag($matches[3],$values);
 
 			} else {
-				$o = Factory::create($attribute,$value);
+				$o = Factory::create($attribute,Arr::get($this->original,$attribute,[]));
 			}
 
 			if (! $result->has($attribute)) {
 				// Set the rdn flag
-				if (preg_match('/^'.$attribute.'=/i',$this->dn))
-					$o->setRDN();
+				$o->is_rdn = preg_match('/^'.$attribute.'=/i',$this->dn);
 
-				// Store our original value to know if this attribute has changed
-				$o->oldValues(Arr::get($this->original,$attribute,[]));
+				// Store our new values to know if this attribute has changed
+				$o->values = collect($values);
 
 				$result->put($attribute,$o);
 			}

app/Traits/MD5Updates.php

@@ -11,8 +11,8 @@ trait MD5Updates
 {
 	public function isDirty(): bool
 	{
-		foreach ($this->values->diff($this->oldValues) as $key => $value)
-			if (md5(Arr::get($this->oldValues,$key)) !== $value)
+		foreach ($this->values->diff($this->values_old) as $key => $value)
+			if (md5(Arr::get($this->values_old,$key)) !== $value)
 				return TRUE;
 
 		return FALSE;

resources/views/components/attribute/binary/jpegphoto.blade.php

@@ -2,7 +2,7 @@
 <!-- $o=Binary\JpegPhoto::class -->
 <x-attribute.layout :edit="$edit" :new="false" :o="$o">
 	<table class="table table-borderless p-0 m-0">
-		@foreach (($old ? $o->old_values : $o->values) as $value)
+		@foreach (($old ? $o->values_old : $o->values) as $value)
 			<tr>
 				@switch ($x=$f->buffer($value,FILEINFO_MIME_TYPE))
 					@case('image/jpeg')

resources/views/components/attribute/rdn.blade.php

@@ -1,6 +1,6 @@
 <!-- $o=RDN::class -->
 <x-attribute.layout :edit="$edit ?? FALSE" :new="$new ?? FALSE" :o="$o">
-	@foreach($o->values as $value)
+	@foreach(($o->values->count() ? $o->values : ['']) as $value)
 		@if($edit)
 			<div class="input-group has-validation mb-3">
 				<select class="form-select @error('rdn')is-invalid @enderror" id="rdn" name="rdn">

resources/views/update.blade.php

@@ -29,9 +29,9 @@
 							</thead>
 
 							<tbody>
-							@foreach ($o->getAttributesAsObjects()->filter(fn($item)=>$item->isDirty()) as $key => $oo)
+							@foreach ($o->getObjects()->filter(fn($item)=>$item->isDirty()) as $key => $oo)
 								<tr>
-									<th rowspan="{{ $x=max($oo->values->keys()->max(),$oo->old_values->keys()->max())+1}}">
+									<th rowspan="{{ $x=max($oo->values->keys()->max(),$oo->values_old->keys()->max())+1}}">
 										<abbr title="{{ $oo->description }}">{{ $oo->name }}</abbr>
 									</th>
 									@for($xx=0;$xx<$x;$xx++)